pgsql: Improve access to parallel query from procedural languages.
Improve access to parallel query from procedural languages.
In SQL, the ability to use parallel query was previous contingent on
fcache->readonly_func, which is only set for non-volatile functions;
but the volatility of a function has no bearing on whether queries
inside it can use parallelism. Remove that condition.
SPI_execute and SPI_execute_with_args always run the plan just once,
though not necessarily to completion. Given the changes in commit
691b8d59281b5177f16fe80858df921f77a8e955, it's sensible to pass
CURSOR_OPT_PARALLEL_OK here, so do that. This improves access to
parallelism for any caller that uses these functions to execute
queries. Such callers include plperl, plpython, pltcl, and plpgsql,
though it's not the case that they all use these functions
exclusively.
In plpgsql, allow parallel query for plain SELECT queries (as
opposed to PERFORM, which already worked) and for plain expressions
(which probably won't go through the executor at all, because they
will likely be simple expressions, but if they do then this helps).
Rafia Sabih and Robert Haas, reviewed by Dilip Kumar and Amit Kapila
Discussion: /messages/by-id/CAOGQiiMfJ+4SQwgG=6CVHWoisiU0+7jtXSuiyXBM3y=A=eJzmg@mail.gmail.com
Branch
------
master
Details
-------
http://git.postgresql.org/pg/commitdiff/61c2e1a95f94bb904953a6281ce17a18ac38ee6d
Modified Files
--------------
src/backend/executor/functions.c | 2 +-
src/backend/executor/spi.c | 4 ++--
src/pl/plpgsql/src/pl_exec.c | 30 +++++++++++++++++-------------
3 files changed, 20 insertions(+), 16 deletions(-)
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Robert Haas <rhaas@postgresql.org> writes:
Improve access to parallel query from procedural languages.
Mandrill has been failing since this patch went in, eg
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mandrill&dt=2017-03-25%2021%3A34%3A08
It doesn't seem to be a platform-specific problem: I can duplicate the
failure here by applying same settings mandrill uses, ie build with
-DRANDOMIZE_ALLOCATED_MEMORY and set force_parallel_mode = regress.
I doubt that the bug is directly in that patch; more likely it's
exposed a pre-existing bug in parallel logic related to triggers.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I wrote:
It doesn't seem to be a platform-specific problem: I can duplicate the
failure here by applying same settings mandrill uses, ie build with
-DRANDOMIZE_ALLOCATED_MEMORY and set force_parallel_mode = regress.
Oh ... scratch that: you don't even need -DRANDOMIZE_ALLOCATED_MEMORY.
For some reason I just assumed that any parallelism-related patch
would have been tested with force_parallel_mode = regress. This one
evidently was not.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Mar 26, 2017 at 3:34 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
It doesn't seem to be a platform-specific problem: I can duplicate the
failure here by applying same settings mandrill uses, ie build with
-DRANDOMIZE_ALLOCATED_MEMORY and set force_parallel_mode = regress.Oh ... scratch that: you don't even need -DRANDOMIZE_ALLOCATED_MEMORY.
For some reason I just assumed that any parallelism-related patch
would have been tested with force_parallel_mode = regress. This one
evidently was not.regards, tom lane
This is caused because trigger related functions are marked safe and
using global variables, hence when executed in parallel are giving
incorrect output. Attached patch fixes this. I have modified only
those functions that are showing incorrect behaviour in the regress
output and checked regression test with force_parallel_mode = regress
and all testcases are passing now.
This concerns me that should we be checking all the system defined
functions once again if they are actually parallel safe...?
--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/
Attachments:
system_defined_fn_update.patchapplication/octet-stream; name=system_defined_fn_update.patchDownload
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 79f9b9012e..5dcec48f63 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -2959,7 +2959,7 @@ DESCR("statistics: reset collected statistics for a single table or index in the
DATA(insert OID = 3777 ( pg_stat_reset_single_function_counters PGNSP PGUID 12 1 0 0 0 f f f f t f v s 1 0 2278 "26" _null_ _null_ _null_ _null_ _null_ pg_stat_reset_single_function_counters _null_ _null_ _null_ ));
DESCR("statistics: reset collected statistics for a single function in the current database");
-DATA(insert OID = 3163 ( pg_trigger_depth PGNSP PGUID 12 1 0 0 0 f f f f t f s s 0 0 23 "" _null_ _null_ _null_ _null_ _null_ pg_trigger_depth _null_ _null_ _null_ ));
+DATA(insert OID = 3163 ( pg_trigger_depth PGNSP PGUID 12 1 0 0 0 f f f f t f s u 0 0 23 "" _null_ _null_ _null_ _null_ _null_ pg_trigger_depth _null_ _null_ _null_ ));
DESCR("current trigger depth");
DATA(insert OID = 3778 ( pg_tablespace_location PGNSP PGUID 12 1 0 0 0 f f f f t f s s 1 0 25 "26" _null_ _null_ _null_ _null_ _null_ pg_tablespace_location _null_ _null_ _null_ ));
@@ -5264,9 +5264,9 @@ DESCR("emit a binary logical decoding message");
/* event triggers */
DATA(insert OID = 3566 ( pg_event_trigger_dropped_objects PGNSP PGUID 12 10 100 0 0 f f f f t t s s 0 0 2249 "" "{26,26,23,16,16,16,25,25,25,25,1009,1009}" "{o,o,o,o,o,o,o,o,o,o,o,o}" "{classid, objid, objsubid, original, normal, is_temporary, object_type, schema_name, object_name, object_identity, address_names, address_args}" _null_ _null_ pg_event_trigger_dropped_objects _null_ _null_ _null_ ));
DESCR("list objects dropped by the current command");
-DATA(insert OID = 4566 ( pg_event_trigger_table_rewrite_oid PGNSP PGUID 12 1 0 0 0 f f f f t f s s 0 0 26 "" "{26}" "{o}" "{oid}" _null_ _null_ pg_event_trigger_table_rewrite_oid _null_ _null_ _null_ ));
+DATA(insert OID = 4566 ( pg_event_trigger_table_rewrite_oid PGNSP PGUID 12 1 0 0 0 f f f f t f s u 0 0 26 "" "{26}" "{o}" "{oid}" _null_ _null_ pg_event_trigger_table_rewrite_oid _null_ _null_ _null_ ));
DESCR("return Oid of the table getting rewritten");
-DATA(insert OID = 4567 ( pg_event_trigger_table_rewrite_reason PGNSP PGUID 12 1 0 0 0 f f f f t f s s 0 0 23 "" _null_ _null_ _null_ _null_ _null_ pg_event_trigger_table_rewrite_reason _null_ _null_ _null_ ));
+DATA(insert OID = 4567 ( pg_event_trigger_table_rewrite_reason PGNSP PGUID 12 1 0 0 0 f f f f t f s u 0 0 23 "" _null_ _null_ _null_ _null_ _null_ pg_event_trigger_table_rewrite_reason _null_ _null_ _null_ ));
DESCR("return reason code for table getting rewritten");
DATA(insert OID = 4568 ( pg_event_trigger_ddl_commands PGNSP PGUID 12 10 100 0 0 f f f f t t s s 0 0 2249 "" "{26,26,23,25,25,25,25,16,32}" "{o,o,o,o,o,o,o,o,o}" "{classid, objid, objsubid, command_tag, object_type, schema_name, object_identity, in_extension, command}" _null_ _null_ pg_event_trigger_ddl_commands _null_ _null_ _null_ ));
DESCR("list DDL actions being executed by the current command");
On Mon, Mar 27, 2017 at 1:48 AM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:
This is caused because trigger related functions are marked safe and
using global variables, hence when executed in parallel are giving
incorrect output. Attached patch fixes this. I have modified only
those functions that are showing incorrect behaviour in the regress
output and checked regression test with force_parallel_mode = regress
and all testcases are passing now.
If it's just that they are relying on unsynchronized global variables,
then it's sufficient to mark them parallel-restricted ('r'). Do we
really need to go all the way to parallel-unsafe ('u')?
This concerns me that should we be checking all the system defined
functions once again if they are actually parallel safe...?
It's certainly possible that we've made mistakes in other places, too.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Mar 27, 2017 at 1:48 AM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:This is caused because trigger related functions are marked safe and
using global variables, hence when executed in parallel are giving
incorrect output.
If it's just that they are relying on unsynchronized global variables,
then it's sufficient to mark them parallel-restricted ('r'). Do we
really need to go all the way to parallel-unsafe ('u')?
Color me confused, but under what circumstances would triggers get
executed by a parallel worker at all? I thought we did not allow
updating queries to be parallelized.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Mar 27, 2017 at 9:25 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Mar 27, 2017 at 1:48 AM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:This is caused because trigger related functions are marked safe and
using global variables, hence when executed in parallel are giving
incorrect output.If it's just that they are relying on unsynchronized global variables,
then it's sufficient to mark them parallel-restricted ('r'). Do we
really need to go all the way to parallel-unsafe ('u')?Color me confused, but under what circumstances would triggers get
executed by a parallel worker at all? I thought we did not allow
updating queries to be parallelized.
Right, we don't. But if the updating query fires a trigger, and the
trigger runs an SQL statement that is itself safe for parallelism,
*that* statement could be run in parallel. In almost all cases it
won't make sense because triggers aren't likely to contain SQL
statements that are expensive enough to justify running them in
parallel, but there's no a priori reason to disallow it.
(And, indeed, I think this commit and the subsequent breakage shows
the value of making sure that parallel query is allowed in as many
places as possible. Running the regression tests with
force_parallel_mode=regress is the best automated tool we have to find
cases where functions are labeled as being more safe than they
actually are, but those tests only try inserting the invisible
single-copy Gather node in cases where parallel query is allowable at
all.)
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Mar 27, 2017 at 5:54 PM, Robert Haas <robertmhaas@gmail.com> wrote:
If it's just that they are relying on unsynchronized global variables,
then it's sufficient to mark them parallel-restricted ('r'). Do we
really need to go all the way to parallel-unsafe ('u')?
Done.
--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/
Attachments:
system_defined_fn_update_v2.patchapplication/octet-stream; name=system_defined_fn_update_v2.patchDownload
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 79f9b9012e..43583de135 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -2959,7 +2959,7 @@ DESCR("statistics: reset collected statistics for a single table or index in the
DATA(insert OID = 3777 ( pg_stat_reset_single_function_counters PGNSP PGUID 12 1 0 0 0 f f f f t f v s 1 0 2278 "26" _null_ _null_ _null_ _null_ _null_ pg_stat_reset_single_function_counters _null_ _null_ _null_ ));
DESCR("statistics: reset collected statistics for a single function in the current database");
-DATA(insert OID = 3163 ( pg_trigger_depth PGNSP PGUID 12 1 0 0 0 f f f f t f s s 0 0 23 "" _null_ _null_ _null_ _null_ _null_ pg_trigger_depth _null_ _null_ _null_ ));
+DATA(insert OID = 3163 ( pg_trigger_depth PGNSP PGUID 12 1 0 0 0 f f f f t f s r 0 0 23 "" _null_ _null_ _null_ _null_ _null_ pg_trigger_depth _null_ _null_ _null_ ));
DESCR("current trigger depth");
DATA(insert OID = 3778 ( pg_tablespace_location PGNSP PGUID 12 1 0 0 0 f f f f t f s s 1 0 25 "26" _null_ _null_ _null_ _null_ _null_ pg_tablespace_location _null_ _null_ _null_ ));
@@ -5264,9 +5264,9 @@ DESCR("emit a binary logical decoding message");
/* event triggers */
DATA(insert OID = 3566 ( pg_event_trigger_dropped_objects PGNSP PGUID 12 10 100 0 0 f f f f t t s s 0 0 2249 "" "{26,26,23,16,16,16,25,25,25,25,1009,1009}" "{o,o,o,o,o,o,o,o,o,o,o,o}" "{classid, objid, objsubid, original, normal, is_temporary, object_type, schema_name, object_name, object_identity, address_names, address_args}" _null_ _null_ pg_event_trigger_dropped_objects _null_ _null_ _null_ ));
DESCR("list objects dropped by the current command");
-DATA(insert OID = 4566 ( pg_event_trigger_table_rewrite_oid PGNSP PGUID 12 1 0 0 0 f f f f t f s s 0 0 26 "" "{26}" "{o}" "{oid}" _null_ _null_ pg_event_trigger_table_rewrite_oid _null_ _null_ _null_ ));
+DATA(insert OID = 4566 ( pg_event_trigger_table_rewrite_oid PGNSP PGUID 12 1 0 0 0 f f f f t f s r 0 0 26 "" "{26}" "{o}" "{oid}" _null_ _null_ pg_event_trigger_table_rewrite_oid _null_ _null_ _null_ ));
DESCR("return Oid of the table getting rewritten");
-DATA(insert OID = 4567 ( pg_event_trigger_table_rewrite_reason PGNSP PGUID 12 1 0 0 0 f f f f t f s s 0 0 23 "" _null_ _null_ _null_ _null_ _null_ pg_event_trigger_table_rewrite_reason _null_ _null_ _null_ ));
+DATA(insert OID = 4567 ( pg_event_trigger_table_rewrite_reason PGNSP PGUID 12 1 0 0 0 f f f f t f s r 0 0 23 "" _null_ _null_ _null_ _null_ _null_ pg_event_trigger_table_rewrite_reason _null_ _null_ _null_ ));
DESCR("return reason code for table getting rewritten");
DATA(insert OID = 4568 ( pg_event_trigger_ddl_commands PGNSP PGUID 12 10 100 0 0 f f f f t t s s 0 0 2249 "" "{26,26,23,25,25,25,25,16,32}" "{o,o,o,o,o,o,o,o,o}" "{classid, objid, objsubid, command_tag, object_type, schema_name, object_identity, in_extension, command}" _null_ _null_ pg_event_trigger_ddl_commands _null_ _null_ _null_ ));
DESCR("list DDL actions being executed by the current command");
On Mon, Mar 27, 2017 at 11:57 PM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:
On Mon, Mar 27, 2017 at 5:54 PM, Robert Haas <robertmhaas@gmail.com> wrote:
If it's just that they are relying on unsynchronized global variables,
then it's sufficient to mark them parallel-restricted ('r'). Do we
really need to go all the way to parallel-unsafe ('u')?Done.
OK, but don't pg_event_trigger_dropped_objects and
pg_event_trigger_ddl_commands need the same treatment?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Mar 28, 2017 at 9:05 PM, Robert Haas <robertmhaas@gmail.com> wrote:
OK, but don't pg_event_trigger_dropped_objects and
pg_event_trigger_ddl_commands need the same treatment?
Done.
I was only concentrating on the build farm failure cases, otherwise I
think more work might be required in this direction.
--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/
Attachments:
system_defined_fn_update_v3.patchapplication/octet-stream; name=system_defined_fn_update_v3.patchDownload
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 79f9b9012e..7b8efc4049 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -2959,7 +2959,7 @@ DESCR("statistics: reset collected statistics for a single table or index in the
DATA(insert OID = 3777 ( pg_stat_reset_single_function_counters PGNSP PGUID 12 1 0 0 0 f f f f t f v s 1 0 2278 "26" _null_ _null_ _null_ _null_ _null_ pg_stat_reset_single_function_counters _null_ _null_ _null_ ));
DESCR("statistics: reset collected statistics for a single function in the current database");
-DATA(insert OID = 3163 ( pg_trigger_depth PGNSP PGUID 12 1 0 0 0 f f f f t f s s 0 0 23 "" _null_ _null_ _null_ _null_ _null_ pg_trigger_depth _null_ _null_ _null_ ));
+DATA(insert OID = 3163 ( pg_trigger_depth PGNSP PGUID 12 1 0 0 0 f f f f t f s r 0 0 23 "" _null_ _null_ _null_ _null_ _null_ pg_trigger_depth _null_ _null_ _null_ ));
DESCR("current trigger depth");
DATA(insert OID = 3778 ( pg_tablespace_location PGNSP PGUID 12 1 0 0 0 f f f f t f s s 1 0 25 "26" _null_ _null_ _null_ _null_ _null_ pg_tablespace_location _null_ _null_ _null_ ));
@@ -5262,13 +5262,13 @@ DATA(insert OID = 3578 ( pg_logical_emit_message PGNSP PGUID 12 1 0 0 0 f f f f
DESCR("emit a binary logical decoding message");
/* event triggers */
-DATA(insert OID = 3566 ( pg_event_trigger_dropped_objects PGNSP PGUID 12 10 100 0 0 f f f f t t s s 0 0 2249 "" "{26,26,23,16,16,16,25,25,25,25,1009,1009}" "{o,o,o,o,o,o,o,o,o,o,o,o}" "{classid, objid, objsubid, original, normal, is_temporary, object_type, schema_name, object_name, object_identity, address_names, address_args}" _null_ _null_ pg_event_trigger_dropped_objects _null_ _null_ _null_ ));
+DATA(insert OID = 3566 ( pg_event_trigger_dropped_objects PGNSP PGUID 12 10 100 0 0 f f f f t t s r 0 0 2249 "" "{26,26,23,16,16,16,25,25,25,25,1009,1009}" "{o,o,o,o,o,o,o,o,o,o,o,o}" "{classid, objid, objsubid, original, normal, is_temporary, object_type, schema_name, object_name, object_identity, address_names, address_args}" _null_ _null_ pg_event_trigger_dropped_objects _null_ _null_ _null_ ));
DESCR("list objects dropped by the current command");
-DATA(insert OID = 4566 ( pg_event_trigger_table_rewrite_oid PGNSP PGUID 12 1 0 0 0 f f f f t f s s 0 0 26 "" "{26}" "{o}" "{oid}" _null_ _null_ pg_event_trigger_table_rewrite_oid _null_ _null_ _null_ ));
+DATA(insert OID = 4566 ( pg_event_trigger_table_rewrite_oid PGNSP PGUID 12 1 0 0 0 f f f f t f s r 0 0 26 "" "{26}" "{o}" "{oid}" _null_ _null_ pg_event_trigger_table_rewrite_oid _null_ _null_ _null_ ));
DESCR("return Oid of the table getting rewritten");
-DATA(insert OID = 4567 ( pg_event_trigger_table_rewrite_reason PGNSP PGUID 12 1 0 0 0 f f f f t f s s 0 0 23 "" _null_ _null_ _null_ _null_ _null_ pg_event_trigger_table_rewrite_reason _null_ _null_ _null_ ));
+DATA(insert OID = 4567 ( pg_event_trigger_table_rewrite_reason PGNSP PGUID 12 1 0 0 0 f f f f t f s r 0 0 23 "" _null_ _null_ _null_ _null_ _null_ pg_event_trigger_table_rewrite_reason _null_ _null_ _null_ ));
DESCR("return reason code for table getting rewritten");
-DATA(insert OID = 4568 ( pg_event_trigger_ddl_commands PGNSP PGUID 12 10 100 0 0 f f f f t t s s 0 0 2249 "" "{26,26,23,25,25,25,25,16,32}" "{o,o,o,o,o,o,o,o,o}" "{classid, objid, objsubid, command_tag, object_type, schema_name, object_identity, in_extension, command}" _null_ _null_ pg_event_trigger_ddl_commands _null_ _null_ _null_ ));
+DATA(insert OID = 4568 ( pg_event_trigger_ddl_commands PGNSP PGUID 12 10 100 0 0 f f f f t t s r 0 0 2249 "" "{26,26,23,25,25,25,25,16,32}" "{o,o,o,o,o,o,o,o,o}" "{classid, objid, objsubid, command_tag, object_type, schema_name, object_identity, in_extension, command}" _null_ _null_ pg_event_trigger_ddl_commands _null_ _null_ _null_ ));
DESCR("list DDL actions being executed by the current command");
/* generic transition functions for ordered-set aggregates */
On Wed, Mar 29, 2017 at 12:02 AM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:
On Tue, Mar 28, 2017 at 9:05 PM, Robert Haas <robertmhaas@gmail.com> wrote:
OK, but don't pg_event_trigger_dropped_objects and
pg_event_trigger_ddl_commands need the same treatment?Done.
I was only concentrating on the build farm failure cases, otherwise I
think more work might be required in this direction.
Sure, but there's some happy medium between checking every line in
pg_proc.h for an error and checking nothing other than the functions
immediately. In this case, there's a group of 4 similarly-named
functions with a similar problem and you changed only the middle two.
Trying to audit the entire file for other mistakes is probably a
fruitless response to this discovery, but auditing the other functions
defined in the same file and with the same naming pattern as the one
you changed seems like something you should do.
Anyway, thanks for debugging this. Committed this version.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers