Enabling parallelism for queries coming from SQL or other PL functions
Hello everybody,
In the current version, queries in SQL or PL functions can not
leverage parallelism. To relax this restriction I prepared a patch,
the approach used in the patch is explained next,
Approach:
1. Allow parallelism for queries in PL functions by passing
CURSOR_OPT_PARALLEL_OK instead of 0 to exec_prepare_plan called from
exec_stmt_execsql or exec_stmt_dynexecute. Similarly, pass
CURSOR_OPT_PARALLEL_OK instead of 0 to SPI_execute and exec_run_select
called from exec_stmt_dynexecute. CURSOR_OPT_PARALLEL_OK is passed to
these functions after checking if the statement is not trigger, since
in that case using parallelism may not be efficient.
2. In ExecutePlan there is an additional check to see if the query is
coming from SQL or PL functions and is having a parallel plan. In that
case we ignore the check of numberTuples since it is always one for
these functions and existing checks restrict parallelism for these
cases. Though, I understand this may not be the most elegant way to do
this, and would be pleased to know some better alternative.
I have attached a sql file containing cases for some pgpsql, perl,
python functions and an .out file which contains the parallel plans
for the queries in these functions after this patch. This might be
helpful in understanding the level of parallelism this patch is
relaxing for PL functions.
Thanks to my colleagues Amit Kapila and Dilip Kumar for discussions in
this regard.
--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/
Attachments:
pl_parallel_v1.patchapplication/octet-stream; name=pl_parallel_v1.patchDownload
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index a666391..fc5c82f 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1596,6 +1596,22 @@ ExecutePlan(EState *estate,
EnterParallelMode();
/*
+ * For SQL or other PL functions, the supplied numberTuples is always
+ * one, thus, upper condition restricts parallelism for the queries
+ * coming from these functions. To enable such parallelism, we ignore the
+ * numberTuples check and identify if the planner considers it safe to use
+ * parallelism for this query and it is coming only from PL or SQL
+ * functions.
+ */
+
+ else if (estate->es_plannedstmt->parallelModeNeeded &&
+ (dest->mydest == DestSPI || dest->mydest == DestSQLFunction))
+ {
+ use_parallel_mode = true;
+ EnterParallelMode();
+ }
+
+ /*
* Loop until we've processed the proper number of tuples from the plan.
*/
for (;;)
@@ -1660,7 +1676,16 @@ ExecutePlan(EState *estate,
*/
current_tuple_count++;
if (numberTuples && numberTuples == current_tuple_count)
+ {
+ /*
+ * For the queries coming from SQL or PL functions, this condition
+ * will be satisfied for the first tuple and since we enabled
+ * parallel workers for it, a proper shutdown of the workers would
+ * be required.
+ */
+ (void) ExecShutdownNode(planstate);
break;
+ }
}
if (use_parallel_mode)
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 55f97b1..45114d2 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -314,7 +314,12 @@ SPI_execute(const char *src, bool read_only, long tcount)
memset(&plan, 0, sizeof(_SPI_plan));
plan.magic = _SPI_PLAN_MAGIC;
- plan.cursor_options = 0;
+
+ /* Allow parallelism if the query is read-only */
+ if(read_only)
+ plan.cursor_options = CURSOR_OPT_PARALLEL_OK;
+ else
+ plan.cursor_options = 0;
_SPI_prepare_oneshot_plan(src, &plan);
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 6fc3db0..27c9e47 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3625,7 +3625,12 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
{
ListCell *l;
- exec_prepare_plan(estate, expr, 0);
+ /* Allow parallelism if the function is not trigger type. */
+ if (estate->func->fn_is_trigger == PLPGSQL_NOT_TRIGGER)
+ exec_prepare_plan(estate, expr, CURSOR_OPT_PARALLEL_OK);
+ else
+ exec_prepare_plan(estate, expr, 0);
+
stmt->mod_stmt = false;
foreach(l, SPI_plan_get_plan_sources(expr->plan))
{
@@ -3869,8 +3874,13 @@ exec_stmt_dynexecute(PLpgSQL_execstate *estate,
estate->readonly_func, 0);
}
else
- exec_res = SPI_execute(querystr, estate->readonly_func, 0);
-
+ {
+ /* Allow parallelism if the function is not trigger type. */
+ if (estate->func->fn_is_trigger == PLPGSQL_NOT_TRIGGER)
+ exec_res = SPI_execute(querystr, estate->readonly_func, CURSOR_OPT_PARALLEL_OK);
+ else
+ exec_res = SPI_execute(querystr, estate->readonly_func, 0);
+ }
switch (exec_res)
{
case SPI_OK_SELECT:
@@ -5172,8 +5182,13 @@ exec_eval_expr(PLpgSQL_execstate *estate,
* If first time through, create a plan for this expression.
*/
if (expr->plan == NULL)
- exec_prepare_plan(estate, expr, 0);
-
+ {
+ /* Allow parallelism is the function is not trigger type. */
+ if (estate->func->fn_is_trigger == PLPGSQL_NOT_TRIGGER)
+ exec_prepare_plan(estate, expr, CURSOR_OPT_PARALLEL_OK);
+ else
+ exec_prepare_plan(estate, expr, 0);
+ }
/*
* If this is a simple expression, bypass SPI and use the executor
* directly
@@ -5183,9 +5198,14 @@ exec_eval_expr(PLpgSQL_execstate *estate,
return result;
/*
- * Else do it the hard way via exec_run_select
+ * Else do it the hard way via exec_run_select. Enable parallelism
+ * if function is not trigger type.
*/
- rc = exec_run_select(estate, expr, 2, NULL, false);
+ if (estate->func->fn_is_trigger == PLPGSQL_NOT_TRIGGER)
+ rc = exec_run_select(estate, expr, 2, NULL, true);
+ else
+ rc = exec_run_select(estate, expr, 2, NULL, false);
+
if (rc != SPI_OK_SELECT)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
On Wed, Feb 22, 2017 at 10:25 AM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:
Hello everybody,
In the current version, queries in SQL or PL functions can not
leverage parallelism. To relax this restriction I prepared a patch,
the approach used in the patch is explained next,
Some initial comments.
----------
if (numberTuples || dest->mydest == DestIntoRel)
use_parallel_mode = false;
if (use_parallel_mode)
EnterParallelMode();
+ else if (estate->es_plannedstmt->parallelModeNeeded &&
+ (dest->mydest == DestSPI || dest->mydest == DestSQLFunction))
+ {
+ use_parallel_mode = true;
+ EnterParallelMode();
+ }I think we can simplify this, can we replace above code with something
like this?
if (dest->mydest == DestIntoRel ||
numberTuples && (dest->mydest != DestSPI || dest->mydest !=
DestSQLFunction))
use_parallel_mode = false;
-------------
+ {
+ /* Allow parallelism if the function is not trigger type. */
+ if (estate->func->fn_is_trigger == PLPGSQL_NOT_TRIGGER)
+ exec_res = SPI_execute(querystr, estate->readonly_func,
CURSOR_OPT_PARALLEL_OK);
+ else
+ exec_res = SPI_execute(querystr, estate->readonly_func, 0);
+ }The last parameter of SPI_execute is tuple count, not cursorOption,
you need to fix this. Also, this is crossing the 80 line boundary.
-----------
Any specific reason for not changing SPI_execute_with_args, EXECUTE
with USING will take this path.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Feb 22, 2017 at 10:22 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
Some initial comments.
----------
if (numberTuples || dest->mydest == DestIntoRel)
use_parallel_mode = false;if (use_parallel_mode) EnterParallelMode(); + else if (estate->es_plannedstmt->parallelModeNeeded && + (dest->mydest == DestSPI || dest->mydest == DestSQLFunction)) + { + use_parallel_mode = true; + EnterParallelMode(); + }I think we can simplify this, can we replace above code with something
like this?if (dest->mydest == DestIntoRel ||
numberTuples && (dest->mydest != DestSPI || dest->mydest !=
DestSQLFunction))
use_parallel_mode = false;
Yes, it can be simplified to
if (dest->mydest == DestIntoRel || (numberTuples && (dest->mydest !=
DestSPI && dest->mydest ! DestSQLFunction)))
Thanks.
-------------
+ { + /* Allow parallelism if the function is not trigger type. */ + if (estate->func->fn_is_trigger == PLPGSQL_NOT_TRIGGER) + exec_res = SPI_execute(querystr, estate->readonly_func, CURSOR_OPT_PARALLEL_OK); + else + exec_res = SPI_execute(querystr, estate->readonly_func, 0); + }The last parameter of SPI_execute is tuple count, not cursorOption,
you need to fix this. Also, this is crossing the 80 line boundary.
Oops, corrected.
-----------
Any specific reason for not changing SPI_execute_with_args, EXECUTE
with USING will take this path.
Fixed.
--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/
Attachments:
pl_parallel_v2.patchapplication/octet-stream; name=pl_parallel_v2.patchDownload
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 3f76a40..f9f74ec 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1589,9 +1589,14 @@ ExecutePlan(EState *estate,
* If a tuple count was supplied, we must force the plan to run without
* parallelism, because we might exit early. Also disable parallelism
* when writing into a relation, because no database changes are allowed
- * in parallel mode.
+ * in parallel mode. For SQL or other PL functions, the supplied tuple
+ * count is always one, To enable parallelism for queries coming from SQL
+ * or other PL functions, we enable parallelism if the planner considers
+ * it safe.
*/
- if (numberTuples || dest->mydest == DestIntoRel)
+
+ if (dest->mydest == DestIntoRel || numberTuples &&
+ (dest->mydest != DestSPI && dest->mydest != DestSQLFunction))
use_parallel_mode = false;
if (use_parallel_mode)
@@ -1662,7 +1667,16 @@ ExecutePlan(EState *estate,
*/
current_tuple_count++;
if (numberTuples && numberTuples == current_tuple_count)
+ {
+ /*
+ * For the queries coming from SQL or PL functions, this condition
+ * will be satisfied for the first tuple and since we enabled
+ * parallel workers for it, a proper shutdown of the workers would
+ * be required.
+ */
+ (void) ExecShutdownNode(planstate);
break;
+ }
}
if (use_parallel_mode)
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 55f97b1..03f6771 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -314,7 +314,12 @@ SPI_execute(const char *src, bool read_only, long tcount)
memset(&plan, 0, sizeof(_SPI_plan));
plan.magic = _SPI_PLAN_MAGIC;
- plan.cursor_options = 0;
+
+ /* Allow parallelism if the query is read-only */
+ if (read_only)
+ plan.cursor_options = CURSOR_OPT_PARALLEL_OK;
+ else
+ plan.cursor_options = 0;
_SPI_prepare_oneshot_plan(src, &plan);
@@ -458,7 +463,12 @@ SPI_execute_with_args(const char *src,
memset(&plan, 0, sizeof(_SPI_plan));
plan.magic = _SPI_PLAN_MAGIC;
- plan.cursor_options = 0;
+
+ /* Allow parallelism if the query is read-only */
+ if (read_only)
+ plan.cursor_options = CURSOR_OPT_PARALLEL_OK;
+ else
+ plan.cursor_options = 0;
plan.nargs = nargs;
plan.argtypes = argtypes;
plan.parserSetup = NULL;
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 6fc3db0..b4f462f 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3625,7 +3625,12 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
{
ListCell *l;
- exec_prepare_plan(estate, expr, 0);
+ /* Allow parallelism if the function is not trigger type. */
+ if (estate->func->fn_is_trigger == PLPGSQL_NOT_TRIGGER)
+ exec_prepare_plan(estate, expr, CURSOR_OPT_PARALLEL_OK);
+ else
+ exec_prepare_plan(estate, expr, 0);
+
stmt->mod_stmt = false;
foreach(l, SPI_plan_get_plan_sources(expr->plan))
{
@@ -5172,7 +5177,13 @@ exec_eval_expr(PLpgSQL_execstate *estate,
* If first time through, create a plan for this expression.
*/
if (expr->plan == NULL)
- exec_prepare_plan(estate, expr, 0);
+ {
+ /* Allow parallelism is the function is not trigger type. */
+ if (estate->func->fn_is_trigger == PLPGSQL_NOT_TRIGGER)
+ exec_prepare_plan(estate, expr, CURSOR_OPT_PARALLEL_OK);
+ else
+ exec_prepare_plan(estate, expr, 0);
+ }
/*
* If this is a simple expression, bypass SPI and use the executor
@@ -5183,9 +5194,14 @@ exec_eval_expr(PLpgSQL_execstate *estate,
return result;
/*
- * Else do it the hard way via exec_run_select
+ * Else do it the hard way via exec_run_select. Enable parallelism if
+ * function is not trigger type.
*/
- rc = exec_run_select(estate, expr, 2, NULL, false);
+ if (estate->func->fn_is_trigger == PLPGSQL_NOT_TRIGGER)
+ rc = exec_run_select(estate, expr, 2, NULL, true);
+ else
+ rc = exec_run_select(estate, expr, 2, NULL, false);
+
if (rc != SPI_OK_SELECT)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
On Thu, Feb 23, 2017 at 12:11 PM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:
Yes, it can be simplified to
if (dest->mydest == DestIntoRel || (numberTuples && (dest->mydest !=
DestSPI && dest->mydest ! DestSQLFunction)))
Thanks.
Okay, this looks cleaner.
Few more comments.
1.I don't see any check in the code which will prevent the parallel
execution of the query inside a function if its called from a DML
statement.
e.g. If we use a function in the update statement's which has the
select statement.
2. How are you protecting, if the outer select is running in parallel,
then the function called from there should not run anything in
parallel? This may allow worker launching another set of workers. Am
I missing something?
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Feb 23, 2017 at 8:58 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
Few more comments.
1.I don't see any check in the code which will prevent the parallel
execution of the query inside a function if its called from a DML
statement.
e.g. If we use a function in the update statement's which has the
select statement.
Having said that, I am thinking do we really need to block such cases?
It just looks fine to me that an update statement calls a function (in
targetlist or condition), which launches a bunch of workers for the
internal query inside PL; finishes the work and shutdown them, only
after this, the update will change any record. So basically I want to
make a point that between the worker launch and shutdown there is no
change in the database state.
Any other opinion on this?
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Feb 23, 2017 at 8:58 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Thu, Feb 23, 2017 at 12:11 PM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:2. How are you protecting, if the outer select is running in parallel,
then the function called from there should not run anything in
parallel? This may allow worker launching another set of workers. Am
I missing something?
We have a below check in standard_planner() (!IsParallelWorker())
which should prohibit generating parallel plan inside worker, if that
is what you are seeing, then we might need a similar check at other
places.
if ((cursorOptions & CURSOR_OPT_PARALLEL_OK) != 0 &&
IsUnderPostmaster &&
dynamic_shared_memory_type != DSM_IMPL_NONE &&
parse->commandType == CMD_SELECT &&
!parse->hasModifyingCTE &&
max_parallel_workers_per_gather > 0 &&
!IsParallelWorker() &&
!IsolationIsSerializable())
{
/* all the cheap tests pass, so scan the query tree */
glob->maxParallelHazard = max_parallel_hazard(parse);
glob->parallelModeOK = (glob->maxParallelHazard != PROPARALLEL_UNSAFE);
}
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Feb 23, 2017 at 9:20 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Thu, Feb 23, 2017 at 8:58 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
Few more comments.
1.I don't see any check in the code which will prevent the parallel
execution of the query inside a function if its called from a DML
statement.
e.g. If we use a function in the update statement's which has the
select statement.Having said that, I am thinking do we really need to block such cases?
It just looks fine to me that an update statement calls a function (in
targetlist or condition), which launches a bunch of workers for the
internal query inside PL; finishes the work and shutdown them, only
after this, the update will change any record. So basically I want to
make a point that between the worker launch and shutdown there is no
change in the database state.
+1. I also think you are right that there should not be a problem in
such a case.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Feb 24, 2017 at 10:06 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
We have a below check in standard_planner() (!IsParallelWorker())
which should prohibit generating parallel plan inside worker, if that
is what you are seeing, then we might need a similar check at other
places.if ((cursorOptions & CURSOR_OPT_PARALLEL_OK) != 0 &&
IsUnderPostmaster &&
dynamic_shared_memory_type != DSM_IMPL_NONE &&
parse->commandType == CMD_SELECT &&
!parse->hasModifyingCTE &&
max_parallel_workers_per_gather > 0 &&
!IsParallelWorker() &&
!IsolationIsSerializable())
{
/* all the cheap tests pass, so scan the query tree */
glob->maxParallelHazard = max_parallel_hazard(parse);
glob->parallelModeOK = (glob->maxParallelHazard != PROPARALLEL_UNSAFE);
}
Ok, I see. But, the problem with PL functions is that plan might have
already generated in previous execution of the function and during
that time outer query might not be running in parallel. So I think we
may need some check during execution time as well?
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Feb 24, 2017 at 11:30 AM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Fri, Feb 24, 2017 at 10:06 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
We have a below check in standard_planner() (!IsParallelWorker())
which should prohibit generating parallel plan inside worker, if that
is what you are seeing, then we might need a similar check at other
places.if ((cursorOptions & CURSOR_OPT_PARALLEL_OK) != 0 &&
IsUnderPostmaster &&
dynamic_shared_memory_type != DSM_IMPL_NONE &&
parse->commandType == CMD_SELECT &&
!parse->hasModifyingCTE &&
max_parallel_workers_per_gather > 0 &&
!IsParallelWorker() &&
!IsolationIsSerializable())
{
/* all the cheap tests pass, so scan the query tree */
glob->maxParallelHazard = max_parallel_hazard(parse);
glob->parallelModeOK = (glob->maxParallelHazard != PROPARALLEL_UNSAFE);
}Ok, I see. But, the problem with PL functions is that plan might have
already generated in previous execution of the function and during
that time outer query might not be running in parallel.
Sure, but that should only happen if the function is *not* declared as
parallel safe (aka in parallel safe functions, we should not generate
parallel plans).
So I think we
may need some check during execution time as well?
Right, I also think we need some mechanism where if the user has not
marked the parallel safe functions appropriately, then such executions
should result in error. For example, if parallel-safe function calls
a parallel-unsafe function which contains either write statement or
statement that could generate a parallel plan, then we should not
allow execution of such queries. We already have safeguard checks at
most places like write statements (see heap_update), however, I think
we need a similar check in ExecGather.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Feb 25, 2017 at 5:12 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
Sure, but that should only happen if the function is *not* declared as
parallel safe (aka in parallel safe functions, we should not generate
parallel plans).
So basically we want to put a restriction that parallel-safe function
can not use the parallel query? This will work but it seems too
restrictive to me. Because by marking function parallel safe we enable
it to be used with the outer parallel query that is fine. But, that
should not restrict the function from using the parallel query if it's
used with the other outer query which is not having the parallel
plan(or function is executed directly).
So I think we
may need some check during execution time as well?Right, I also think we need some mechanism where if the user has not
marked the parallel safe functions appropriately, then such executions
should result in error. For example, if parallel-safe function calls
a parallel-unsafe function which contains either write statement or
statement that could generate a parallel plan, then we should not
allow execution of such queries. We already have safeguard checks at
most places like write statements (see heap_update), however, I think
we need a similar check in ExecGather.
How about we allow parallel-safe functions to create a parallel plan
but whenever it's used from an unsafe place i.e. already in the
parallel mode we don't allow to launch worker?
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Feb 25, 2017 at 9:47 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Sat, Feb 25, 2017 at 5:12 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
Sure, but that should only happen if the function is *not* declared as
parallel safe (aka in parallel safe functions, we should not generate
parallel plans).So basically we want to put a restriction that parallel-safe function
can not use the parallel query? This will work but it seems too
restrictive to me. Because by marking function parallel safe we enable
it to be used with the outer parallel query that is fine. But, that
should not restrict the function from using the parallel query if it's
used with the other outer query which is not having the parallel
plan(or function is executed directly).
I think if the user is explicitly marking a function as parallel-safe,
then it doesn't make much sense to allow parallel query in such
functions as it won't be feasible for the planner (or at least it will
be quite expensive) to detect the same. By the way, if the user has
any such expectation from a function, then he can mark the function as
parallel-restricted or parallel-unsafe.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
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, Feb 26, 2017 at 6:34 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sat, Feb 25, 2017 at 9:47 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Sat, Feb 25, 2017 at 5:12 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
Sure, but that should only happen if the function is *not* declared as
parallel safe (aka in parallel safe functions, we should not generate
parallel plans).So basically we want to put a restriction that parallel-safe function
can not use the parallel query? This will work but it seems too
restrictive to me. Because by marking function parallel safe we enable
it to be used with the outer parallel query that is fine. But, that
should not restrict the function from using the parallel query if it's
used with the other outer query which is not having the parallel
plan(or function is executed directly).I think if the user is explicitly marking a function as parallel-safe,
then it doesn't make much sense to allow parallel query in such
functions as it won't be feasible for the planner (or at least it will
be quite expensive) to detect the same. By the way, if the user has
any such expectation from a function, then he can mark the function as
parallel-restricted or parallel-unsafe.
However, if a query is parallel-safe, it might not end up getting run
in parallel. In that case, it could still benefit from parallelism
internally. I think we want to allow that. For example, suppose you
run a query like:
SELECT x, sum(somewhat_expensive_function(y)) FROM sometab GROUP BY 1;
If sometab isn't very big, it's probably better to use a non-parallel
plan for this query, because then somewhat_expensive_function() can
still use parallelism internally, which might be better. However, if
sometab is large enough, then it might be better to parallelize the
whole query using a Partial/FinalizeAggregate and force each call to
somewhat_expensive_function() to run serially. So I don't think a
hard-and-fast rule that parallel-safe functions can't use parallelism
internally is a good idea.
--
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 Wed, Feb 22, 2017 at 10:25 AM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:
1. Allow parallelism for queries in PL functions by passing
CURSOR_OPT_PARALLEL_OK instead of 0 to exec_prepare_plan called from
exec_stmt_execsql or exec_stmt_dynexecute. Similarly, pass
CURSOR_OPT_PARALLEL_OK instead of 0 to SPI_execute and exec_run_select
called from exec_stmt_dynexecute. CURSOR_OPT_PARALLEL_OK is passed to
these functions after checking if the statement is not trigger, since
in that case using parallelism may not be efficient.2. In ExecutePlan there is an additional check to see if the query is
coming from SQL or PL functions and is having a parallel plan. In that
case we ignore the check of numberTuples since it is always one for
these functions and existing checks restrict parallelism for these
cases. Though, I understand this may not be the most elegant way to do
this, and would be pleased to know some better alternative.
I think I see the problem that you're trying to solve, but I agree
that this doesn't seem all that elegant. The reason why we have that
numberTuples check is because we're afraid that we might be in a
context like the extended-query protocol, where the caller can ask for
1 tuple, and then later ask for another tuple. That won't work,
because once we shut down the workers we can't reliably generate the
rest of the query results. However, I think it would probably work
fine to let somebody ask for less than the full number of tuples if
it's certain that they won't later ask for any more.
So maybe what we ought to do is allow CURSOR_OPT_PARALLEL_OK to be set
any time we know that ExecutorRun() will be called for the QueryDesc
at most once rather than (as at present) only where we know it will be
executed only once with a tuple-count of zero. Then we could change
things in ExecutePlan so that it doesn't disable parallel query when
the tuple-count is non-zero, but does take an extra argument "bool
execute_only_once", and it disables parallel execution if that is not
true. Also, if ExecutorRun() is called a second time for the same
QueryDesc when execute_only_once is specified as true, it should
elog(ERROR, ...). Then exec_execute_message(), for example, can pass
that argument as false when the tuple-count is non-zero, but other
places that are going to fetch a limited number of rows could pass it
as true even though they also pass a row-count.
I'm not sure if that's exactly right, but something along those lines
seems like it should work.
I think that a final patch for this functionality should involve
adding CURSOR_OPT_PARALLEL_OK to appropriate places in each PL, plus
maybe some infrastructure changes like the ones mentioned above.
Maybe it can be divided into two patches, one to make the
infrastructure changes and a second to add CURSOR_OPT_PARALLEL_OK to
more places.
+ /* Allow parallelism if the query is read-only */
+ if(read_only)
+ plan.cursor_options = CURSOR_OPT_PARALLEL_OK;
+ else
+ plan.cursor_options = 0;I don't think this can ever be right. If the only thing we are
worried about is the query being read-only, then we could just pass
CURSOR_OPT_PARALLEL_OK everywhere and planner.c would figure it out
without any help from us. But that's not the problem. The problem is
that the PL may be using a function like SPI_prepare_cursor(), where
it's going to later use SPI_cursor_fetch() or similar. Parallel query
can't handle cursor operations. Whatever changes we make to spi.c
should involve passing CURSOR_OPT_PARALLEL_OK everywhere that we can
be sure there will be no cursor operations and not anywhere that we
might have cursor operations. Cursor operations - or more
specifically anything that might try to suspend execution of the query
and resume later - are the problem. Things that will already cause
the tests in standard_planner() to disable parallelism don't need to
be rechecked elsewhere:
if ((cursorOptions & CURSOR_OPT_PARALLEL_OK) != 0 &&
IsUnderPostmaster &&
dynamic_shared_memory_type != DSM_IMPL_NONE &&
parse->commandType == CMD_SELECT &&
!parse->hasModifyingCTE &&
max_parallel_workers_per_gather > 0 &&
!IsParallelWorker() &&
!IsolationIsSerializable())
{
/* all the cheap tests pass, so scan the query tree */
glob->maxParallelHazard = max_parallel_hazard(parse);
glob->parallelModeOK = (glob->maxParallelHazard != PROPARALLEL_UNSAFE);
}
else
{
/* skip the query tree scan, just assume it's unsafe */
glob->maxParallelHazard = PROPARALLEL_UNSAFE;
glob->parallelModeOK = false;
}
--
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 Sun, Feb 26, 2017 at 4:14 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Sun, Feb 26, 2017 at 6:34 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sat, Feb 25, 2017 at 9:47 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Sat, Feb 25, 2017 at 5:12 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
Sure, but that should only happen if the function is *not* declared as
parallel safe (aka in parallel safe functions, we should not generate
parallel plans).So basically we want to put a restriction that parallel-safe function
can not use the parallel query? This will work but it seems too
restrictive to me. Because by marking function parallel safe we enable
it to be used with the outer parallel query that is fine. But, that
should not restrict the function from using the parallel query if it's
used with the other outer query which is not having the parallel
plan(or function is executed directly).I think if the user is explicitly marking a function as parallel-safe,
then it doesn't make much sense to allow parallel query in such
functions as it won't be feasible for the planner (or at least it will
be quite expensive) to detect the same. By the way, if the user has
any such expectation from a function, then he can mark the function as
parallel-restricted or parallel-unsafe.However, if a query is parallel-safe, it might not end up getting run
in parallel. In that case, it could still benefit from parallelism
internally. I think we want to allow that. For example, suppose you
run a query like:SELECT x, sum(somewhat_expensive_function(y)) FROM sometab GROUP BY 1;
If sometab isn't very big, it's probably better to use a non-parallel
plan for this query, because then somewhat_expensive_function() can
still use parallelism internally, which might be better. However, if
sometab is large enough, then it might be better to parallelize the
whole query using a Partial/FinalizeAggregate and force each call to
somewhat_expensive_function() to run serially.
Is there any easy way to find out which way is less expensive? Even
if we find some way or just make a rule that when an outer query uses
parallelism, then force function call to run serially, how do we
achieve that? I mean in each worker we can ensure that each
individual statements from a function can run serially (by having a
check of isparallelworker() in gather node), but having a similar
check in the master backend is tricky or maybe we don't want to care
for the same in master backend. Do you have any suggestions on how to
make it work?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
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, Feb 27, 2017 at 8:33 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
Is there any easy way to find out which way is less expensive?
No. But that's a separate problem. I'm just saying we shouldn't
arbitrarily prohibit parallelism for parallel-safe functions.
Even
if we find some way or just make a rule that when an outer query uses
parallelism, then force function call to run serially, how do we
achieve that I mean in each worker we can ensure that each
individual statements from a function can run serially (by having a
check of isparallelworker() in gather node), but having a similar
check in the master backend is tricky or maybe we don't want to care
for the same in master backend. Do you have any suggestions on how to
make it work?
I don't understand what's wrong with the existing logic in standard_planner().
--
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, Feb 27, 2017 at 12:21 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Feb 27, 2017 at 8:33 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
Is there any easy way to find out which way is less expensive?
No. But that's a separate problem. I'm just saying we shouldn't
arbitrarily prohibit parallelism for parallel-safe functions.Even
if we find some way or just make a rule that when an outer query uses
parallelism, then force function call to run serially, how do we
achieve that I mean in each worker we can ensure that each
individual statements from a function can run serially (by having a
check of isparallelworker() in gather node), but having a similar
check in the master backend is tricky or maybe we don't want to care
for the same in master backend. Do you have any suggestions on how to
make it work?I don't understand what's wrong with the existing logic in standard_planner().
When such a function (that contains statements which have parallel
plans) is being executed as part of another parallel plan, it can
allow spawning workers unboundedly. Assume a query like select *
from t1 where c1 < func1(), this can use parallel scan for t1 and
then in master backend, during partial scan of t1, it can again spawn
new set of workers for queries inside func1(), this can happen
multiple times if parallel query inside func1() again calls some other
function func2() which has parallel query. Now, this might be okay,
but today such a situation doesn't exist that Gather execution can
invoke another Gather node, so it is worth to consider if we want to
allow it.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
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, Feb 28, 2017 at 5:25 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
When such a function (that contains statements which have parallel
plans) is being executed as part of another parallel plan, it can
allow spawning workers unboundedly. Assume a query like select *
from t1 where c1 < func1(), this can use parallel scan for t1 and
then in master backend, during partial scan of t1, it can again spawn
new set of workers for queries inside func1(), this can happen
multiple times if parallel query inside func1() again calls some other
function func2() which has parallel query. Now, this might be okay,
but today such a situation doesn't exist that Gather execution can
invoke another Gather node, so it is worth to consider if we want to
allow it.
If we want to prohibit that, the check in standard_planner can be
changed from !IsParallelWorker() to !IsInParallelMode(), but I'm not
100% sure whether that's an improvement or not. I would be inclined
to leave it alone unless we get several votes to change it.
--
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 Thu, Mar 2, 2017 at 3:50 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Feb 28, 2017 at 5:25 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
When such a function (that contains statements which have parallel
plans) is being executed as part of another parallel plan, it can
allow spawning workers unboundedly. Assume a query like select *
from t1 where c1 < func1(), this can use parallel scan for t1 and
then in master backend, during partial scan of t1, it can again spawn
new set of workers for queries inside func1(), this can happen
multiple times if parallel query inside func1() again calls some other
function func2() which has parallel query. Now, this might be okay,
but today such a situation doesn't exist that Gather execution can
invoke another Gather node, so it is worth to consider if we want to
allow it.If we want to prohibit that, the check in standard_planner can be
changed from !IsParallelWorker() to !IsInParallelMode(), but I'm not
100% sure whether that's an improvement or not.
I am not sure how you can achieve that by just changing
standard_planner() code, because the plans of statements inside pl can
be cached in which case it will not try to regenerate the plan.
I would be inclined
to leave it alone unless we get several votes to change it.
Okay, not a problem.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Mar 2, 2017 at 3:54 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Mar 2, 2017 at 3:50 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Feb 28, 2017 at 5:25 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
When such a function (that contains statements which have parallel
plans) is being executed as part of another parallel plan, it can
allow spawning workers unboundedly. Assume a query like select *
from t1 where c1 < func1(), this can use parallel scan for t1 and
then in master backend, during partial scan of t1, it can again spawn
new set of workers for queries inside func1(), this can happen
multiple times if parallel query inside func1() again calls some other
function func2() which has parallel query. Now, this might be okay,
but today such a situation doesn't exist that Gather execution can
invoke another Gather node, so it is worth to consider if we want to
allow it.If we want to prohibit that, the check in standard_planner can be
changed from !IsParallelWorker() to !IsInParallelMode(), but I'm not
100% sure whether that's an improvement or not.I am not sure how you can achieve that by just changing
standard_planner() code, because the plans of statements inside pl can
be cached in which case it will not try to regenerate the plan.
Oh, good point.
I would be inclined
to leave it alone unless we get several votes to change it.Okay, not a problem.
Cool.
--
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 Sun, Feb 26, 2017 at 7:09 PM, Robert Haas <robertmhaas@gmail.com> wrote:
I think I see the problem that you're trying to solve, but I agree
that this doesn't seem all that elegant. The reason why we have that
numberTuples check is because we're afraid that we might be in a
context like the extended-query protocol, where the caller can ask for
1 tuple, and then later ask for another tuple. That won't work,
because once we shut down the workers we can't reliably generate the
rest of the query results. However, I think it would probably work
fine to let somebody ask for less than the full number of tuples if
it's certain that they won't later ask for any more.So maybe what we ought to do is allow CURSOR_OPT_PARALLEL_OK to be set
any time we know that ExecutorRun() will be called for the QueryDesc
at most once rather than (as at present) only where we know it will be
executed only once with a tuple-count of zero. Then we could change
things in ExecutePlan so that it doesn't disable parallel query when
the tuple-count is non-zero, but does take an extra argument "bool
execute_only_once", and it disables parallel execution if that is not
true. Also, if ExecutorRun() is called a second time for the same
QueryDesc when execute_only_once is specified as true, it should
elog(ERROR, ...). Then exec_execute_message(), for example, can pass
that argument as false when the tuple-count is non-zero, but other
places that are going to fetch a limited number of rows could pass it
as true even though they also pass a row-count.I'm not sure if that's exactly right, but something along those lines
seems like it should work.
IIUC, this needs an additional bool execute_once in the queryDesc which is
set to true in standard_ExecutorRun when the query is detected to be coming
from PL function or provided count is zero i.e. execute till the end, in
case execute_once is already true then report the error.
I think that a final patch for this functionality should involve
adding CURSOR_OPT_PARALLEL_OK to appropriate places in each PL, plus
maybe some infrastructure changes like the ones mentioned above.
Maybe it can be divided into two patches, one to make the
infrastructure changes and a second to add CURSOR_OPT_PARALLEL_OK to
more places.
I have split the patch into two, one is to allow optimiser to select a
parallel plan for queries in PL functions
(pl_parallel_opt_support_v1.patch), wherein CURSOR_OPT_PARALLEL_OK is
passed at required places.
Next, the patch for allowing execution of such queries in parallel mode,
that involves infrastructural changes along the lines mentioned upthread
(pl_parallel_exec_support_v1.patch).
--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/
Attachments:
pl_parallel_exec_support_v1.patchapplication/octet-stream; name=pl_parallel_exec_support_v1.patchDownload
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 3f76a407d7..098b14d227 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -84,7 +84,7 @@ static void ExecutePlan(EState *estate, PlanState *planstate,
bool sendTuples,
uint64 numberTuples,
ScanDirection direction,
- DestReceiver *dest);
+ DestReceiver *dest, bool execute_once);
static bool ExecCheckRTEPerms(RangeTblEntry *rte);
static bool ExecCheckRTEPermsModified(Oid relOid, Oid userid,
Bitmapset *modifiedCols,
@@ -97,7 +97,6 @@ static char *ExecBuildSlotValueDescription(Oid reloid,
int maxfieldlen);
static void EvalPlanQualStart(EPQState *epqstate, EState *parentestate,
Plan *planTree);
-
/*
* Note that GetUpdatedColumns() also exists in commands/trigger.c. There does
* not appear to be any good header to put it into, given the structures that
@@ -340,6 +339,12 @@ standard_ExecutorRun(QueryDesc *queryDesc,
* run plan
*/
if (!ScanDirectionIsNoMovement(direction))
+ {
+ if (queryDesc->execute_once)
+ elog(ERROR, "query is called more than once for the same query-descriptor %s", queryDesc->sourceText);
+ if (dest->mydest == DestSPI || dest->mydest == DestSQLFunction || count == 0)
+ queryDesc->execute_once = true;
+
ExecutePlan(estate,
queryDesc->planstate,
queryDesc->plannedstmt->parallelModeNeeded,
@@ -347,7 +352,9 @@ standard_ExecutorRun(QueryDesc *queryDesc,
sendTuples,
count,
direction,
- dest);
+ dest,
+ queryDesc->execute_once);
+ }
/*
* shutdown tuple receiver, if we started it
@@ -1570,7 +1577,8 @@ ExecutePlan(EState *estate,
bool sendTuples,
uint64 numberTuples,
ScanDirection direction,
- DestReceiver *dest)
+ DestReceiver *dest,
+ bool execute_once)
{
TupleTableSlot *slot;
uint64 current_tuple_count;
@@ -1589,9 +1597,11 @@ ExecutePlan(EState *estate,
* If a tuple count was supplied, we must force the plan to run without
* parallelism, because we might exit early. Also disable parallelism
* when writing into a relation, because no database changes are allowed
- * in parallel mode.
+ * in parallel mode. To enable parallelism for queries coming from SQL
+ * or other PL functions, allow parallelism if the planner considers
+ * it safe and those are coming from queryDesc which will execute only once.
*/
- if (numberTuples || dest->mydest == DestIntoRel)
+ if (!execute_once || dest->mydest == DestIntoRel)
use_parallel_mode = false;
if (use_parallel_mode)
@@ -1662,7 +1672,16 @@ ExecutePlan(EState *estate,
*/
current_tuple_count++;
if (numberTuples && numberTuples == current_tuple_count)
+ {
+ /*
+ * For the queries coming from SQL or PL functions, this condition
+ * will be satisfied for the first tuple and since we enabled
+ * parallel workers for it, a proper shutdown of the workers would
+ * be required.
+ */
+ (void) ExecShutdownNode(planstate);
break;
+ }
}
if (use_parallel_mode)
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index e64ea2ed76..7737bf5041 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -87,6 +87,7 @@ CreateQueryDesc(PlannedStmt *plannedstmt,
qd->estate = NULL;
qd->planstate = NULL;
qd->totaltime = NULL;
+ qd->execute_once = false;
return qd;
}
diff --git a/src/include/executor/execdesc.h b/src/include/executor/execdesc.h
index c99ea81815..062f47fb76 100644
--- a/src/include/executor/execdesc.h
+++ b/src/include/executor/execdesc.h
@@ -49,6 +49,7 @@ typedef struct QueryDesc
/* This is always set NULL by the core system, but plugins can change it */
struct Instrumentation *totaltime; /* total time spent in ExecutorRun */
+ bool execute_once;
} QueryDesc;
/* in pquery.c */
pl_parallel_opt_support_v1.patchapplication/octet-stream; name=pl_parallel_opt_support_v1.patchDownload
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 55f97b14e6..1f6e669327 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -314,7 +314,7 @@ SPI_execute(const char *src, bool read_only, long tcount)
memset(&plan, 0, sizeof(_SPI_plan));
plan.magic = _SPI_PLAN_MAGIC;
- plan.cursor_options = 0;
+ plan.cursor_options = CURSOR_OPT_PARALLEL_OK;
_SPI_prepare_oneshot_plan(src, &plan);
@@ -458,7 +458,7 @@ SPI_execute_with_args(const char *src,
memset(&plan, 0, sizeof(_SPI_plan));
plan.magic = _SPI_PLAN_MAGIC;
- plan.cursor_options = 0;
+ plan.cursor_options = CURSOR_OPT_PARALLEL_OK;
plan.nargs = nargs;
plan.argtypes = argtypes;
plan.parserSetup = NULL;
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 6fc3db07fe..be30a5b744 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3625,7 +3625,12 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
{
ListCell *l;
- exec_prepare_plan(estate, expr, 0);
+ /* Allow parallelism if the function is not trigger type. */
+ if (estate->func->fn_is_trigger == PLPGSQL_NOT_TRIGGER)
+ exec_prepare_plan(estate, expr, CURSOR_OPT_PARALLEL_OK);
+ else
+ exec_prepare_plan(estate, expr, 0);
+
stmt->mod_stmt = false;
foreach(l, SPI_plan_get_plan_sources(expr->plan))
{
@@ -5172,7 +5177,7 @@ exec_eval_expr(PLpgSQL_execstate *estate,
* If first time through, create a plan for this expression.
*/
if (expr->plan == NULL)
- exec_prepare_plan(estate, expr, 0);
+ exec_prepare_plan(estate, expr, CURSOR_OPT_PARALLEL_OK);
/*
* If this is a simple expression, bypass SPI and use the executor
@@ -5183,9 +5188,14 @@ exec_eval_expr(PLpgSQL_execstate *estate,
return result;
/*
- * Else do it the hard way via exec_run_select
+ * Else do it the hard way via exec_run_select. Enable parallelism if
+ * function is not trigger type.
*/
- rc = exec_run_select(estate, expr, 2, NULL, false);
+ if (estate->func->fn_is_trigger == PLPGSQL_NOT_TRIGGER)
+ rc = exec_run_select(estate, expr, 2, NULL, true);
+ else
+ rc = exec_run_select(estate, expr, 2, NULL, false);
+
if (rc != SPI_OK_SELECT)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
On Tue, Mar 7, 2017 at 9:07 AM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:
I have split the patch into two, one is to allow optimiser to select a
parallel plan for queries in PL functions
(pl_parallel_opt_support_v1.patch), wherein CURSOR_OPT_PARALLEL_OK is passed
at required places.Next, the patch for allowing execution of such queries in parallel mode,
that involves infrastructural changes along the lines mentioned upthread
(pl_parallel_exec_support_v1.patch).
Logically, these patches go in the other order: you have to make the
infrastructure changes first, and then after that you can enable
parallelism in the places that are now safe.
I think any patch that bases the determination of whether it's OK to
use parallel query on the DestReceiver type is unacceptable. There's
just no logical reason to believe that every place that uses a certain
set of DestReceiver types is OK and others are not. What matters is
whether the query can ever be executed more than once, and that's got
to be tracked explicitly.
Here's a draft patch showing the sort of thing I have in mind. I
think it needs more work, but it gives you the idea, I hope. This is
loosely based on your pl_parallel_exec_support_v1.patch, but what I've
done here is added some flags that carry the information about whether
there will be only one or maybe more than one call to ExecutorRun to a
bunch of relevant places.
I think this might have the effect of disabling parallel query in some
cases where PL/pgsql currently allows it, and I think that may be
necessary. (We may need to back-patch a different fix into 9.6.)
There are two places where we currently set CURSOR_OPT_PARALLEL_OK in
PL/pgsql: exec_stmt_return_query() sets it when calling
exec_dynquery_with_params(), and exec_run_select() calls it when
calling exec_prepare_plan() if parallelOK is set. The latter is OK,
because exec_run_select() runs the plan via
SPI_execute_plan_with_paramlist(), which calls _SPI_execute_plan(),
which calls _SPI_pquery(). But the former is broken, because
exec_stmt_return_query() executes the query by calling
SPI_cursor_fetch() with a fetch count of 50, and that calls
_SPI_cursor_operation() which calls PortalRunFetch() -- and of course
each call to PortalRunFetch() is going to cause a separate call to
PortalRunSelect(), resulting in a separate call to ExecutorRun().
Oops.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
execute-once.patchapplication/octet-stream; name=execute-once.patchDownload
diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c
index 34b9f15..9213ffb 100644
--- a/contrib/auto_explain/auto_explain.c
+++ b/contrib/auto_explain/auto_explain.c
@@ -61,7 +61,7 @@ void _PG_fini(void);
static void explain_ExecutorStart(QueryDesc *queryDesc, int eflags);
static void explain_ExecutorRun(QueryDesc *queryDesc,
ScanDirection direction,
- uint64 count);
+ uint64 count, bool execute_once);
static void explain_ExecutorFinish(QueryDesc *queryDesc);
static void explain_ExecutorEnd(QueryDesc *queryDesc);
@@ -257,15 +257,16 @@ explain_ExecutorStart(QueryDesc *queryDesc, int eflags)
* ExecutorRun hook: all we need do is track nesting depth
*/
static void
-explain_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, uint64 count)
+explain_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction,
+ uint64 count, bool execute_once)
{
nesting_level++;
PG_TRY();
{
if (prev_ExecutorRun)
- prev_ExecutorRun(queryDesc, direction, count);
+ prev_ExecutorRun(queryDesc, direction, count, execute_once);
else
- standard_ExecutorRun(queryDesc, direction, count);
+ standard_ExecutorRun(queryDesc, direction, count, execute_once);
nesting_level--;
}
PG_CATCH();
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 62dec87..d09a18c 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -290,7 +290,7 @@ static void pgss_post_parse_analyze(ParseState *pstate, Query *query);
static void pgss_ExecutorStart(QueryDesc *queryDesc, int eflags);
static void pgss_ExecutorRun(QueryDesc *queryDesc,
ScanDirection direction,
- uint64 count);
+ uint64 count, bool execute_once);
static void pgss_ExecutorFinish(QueryDesc *queryDesc);
static void pgss_ExecutorEnd(QueryDesc *queryDesc);
static void pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
@@ -871,15 +871,16 @@ pgss_ExecutorStart(QueryDesc *queryDesc, int eflags)
* ExecutorRun hook: all we need do is track nesting depth
*/
static void
-pgss_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, uint64 count)
+pgss_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, uint64 count,
+ bool execute_once)
{
nested_level++;
PG_TRY();
{
if (prev_ExecutorRun)
- prev_ExecutorRun(queryDesc, direction, count);
+ prev_ExecutorRun(queryDesc, direction, count, execute_once);
else
- standard_ExecutorRun(queryDesc, direction, count);
+ standard_ExecutorRun(queryDesc, direction, count, execute_once);
nested_level--;
}
PG_CATCH();
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 3102ab1..45cbf2d 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2067,7 +2067,7 @@ CopyTo(CopyState cstate)
else
{
/* run the plan --- the dest receiver will send tuples */
- ExecutorRun(cstate->queryDesc, ForwardScanDirection, 0L);
+ ExecutorRun(cstate->queryDesc, ForwardScanDirection, 0L, true);
processed = ((DR_copy *) cstate->queryDesc->dest)->processed;
}
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 646a884..3daffc8 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -347,7 +347,7 @@ ExecCreateTableAs(CreateTableAsStmt *stmt, const char *queryString,
ExecutorStart(queryDesc, GetIntoRelEFlags(into));
/* run the plan to completion */
- ExecutorRun(queryDesc, ForwardScanDirection, 0L);
+ ExecutorRun(queryDesc, ForwardScanDirection, 0L, true);
/* save the rowcount if we're given a completionTag to fill */
if (completionTag)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index c9e0a3e..0b59191 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -524,7 +524,7 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
dir = ForwardScanDirection;
/* run the plan */
- ExecutorRun(queryDesc, dir, 0L);
+ ExecutorRun(queryDesc, dir, 0L, true);
/* run cleanup too */
ExecutorFinish(queryDesc);
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 585fcce..e39c647 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -742,7 +742,7 @@ execute_sql_string(const char *sql, const char *filename)
dest, NULL, 0);
ExecutorStart(qdesc, 0);
- ExecutorRun(qdesc, ForwardScanDirection, 0);
+ ExecutorRun(qdesc, ForwardScanDirection, 0, true);
ExecutorFinish(qdesc);
ExecutorEnd(qdesc);
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index a18c917..ad39df2 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -404,7 +404,7 @@ refresh_matview_datafill(DestReceiver *dest, Query *query,
ExecutorStart(queryDesc, EXEC_FLAG_WITHOUT_OIDS);
/* run the plan */
- ExecutorRun(queryDesc, ForwardScanDirection, 0L);
+ ExecutorRun(queryDesc, ForwardScanDirection, 0L, true);
/* and clean up */
ExecutorFinish(queryDesc);
diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c
index 29d0430..f57cf87 100644
--- a/src/backend/commands/portalcmds.c
+++ b/src/backend/commands/portalcmds.c
@@ -395,7 +395,7 @@ PersistHoldablePortal(Portal portal)
true);
/* Fetch the result set into the tuplestore */
- ExecutorRun(queryDesc, ForwardScanDirection, 0L);
+ ExecutorRun(queryDesc, ForwardScanDirection, 0L, false);
(*queryDesc->dest->rDestroy) (queryDesc->dest);
queryDesc->dest = NULL;
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 116ed67..783a666 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -301,7 +301,7 @@ ExecuteQuery(ExecuteStmt *stmt, IntoClause *intoClause,
*/
PortalStart(portal, paramLI, eflags, GetActiveSnapshot());
- (void) PortalRun(portal, count, false, dest, dest, completionTag);
+ (void) PortalRun(portal, count, false, true, dest, dest, completionTag);
PortalDrop(portal, false);
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index f5cd65d..4f5b7b3 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -85,7 +85,8 @@ static void ExecutePlan(EState *estate, PlanState *planstate,
bool sendTuples,
uint64 numberTuples,
ScanDirection direction,
- DestReceiver *dest);
+ DestReceiver *dest,
+ bool execute_once);
static bool ExecCheckRTEPerms(RangeTblEntry *rte);
static bool ExecCheckRTEPermsModified(Oid relOid, Oid userid,
Bitmapset *modifiedCols,
@@ -288,17 +289,18 @@ standard_ExecutorStart(QueryDesc *queryDesc, int eflags)
*/
void
ExecutorRun(QueryDesc *queryDesc,
- ScanDirection direction, uint64 count)
+ ScanDirection direction, uint64 count,
+ bool execute_once)
{
if (ExecutorRun_hook)
- (*ExecutorRun_hook) (queryDesc, direction, count);
+ (*ExecutorRun_hook) (queryDesc, direction, count, execute_once);
else
- standard_ExecutorRun(queryDesc, direction, count);
+ standard_ExecutorRun(queryDesc, direction, count, execute_once);
}
void
standard_ExecutorRun(QueryDesc *queryDesc,
- ScanDirection direction, uint64 count)
+ ScanDirection direction, uint64 count, bool execute_once)
{
EState *estate;
CmdType operation;
@@ -345,6 +347,11 @@ standard_ExecutorRun(QueryDesc *queryDesc,
* run plan
*/
if (!ScanDirectionIsNoMovement(direction))
+ {
+ if (execute_once && queryDesc->already_executed)
+ elog(ERROR, "can't re-execute query flagged for single execution");
+ queryDesc->already_executed = true;
+
ExecutePlan(estate,
queryDesc->planstate,
queryDesc->plannedstmt->parallelModeNeeded,
@@ -352,7 +359,9 @@ standard_ExecutorRun(QueryDesc *queryDesc,
sendTuples,
count,
direction,
- dest);
+ dest,
+ execute_once);
+ }
/*
* shutdown tuple receiver, if we started it
@@ -1575,7 +1584,8 @@ ExecutePlan(EState *estate,
bool sendTuples,
uint64 numberTuples,
ScanDirection direction,
- DestReceiver *dest)
+ DestReceiver *dest,
+ bool execute_once)
{
TupleTableSlot *slot;
uint64 current_tuple_count;
@@ -1591,12 +1601,12 @@ ExecutePlan(EState *estate,
estate->es_direction = direction;
/*
- * If a tuple count was supplied, we must force the plan to run without
- * parallelism, because we might exit early. Also disable parallelism
- * when writing into a relation, because no database changes are allowed
- * in parallel mode.
+ * If the plan might potentially be executed multiple times, we must force
+ * it to run without parallelism, because we might exit early. Also
+ * disable parallelism when writing into a relation, because no database
+ * changes are allowed in parallel mode.
*/
- if (numberTuples || dest->mydest == DestIntoRel)
+ if (!execute_once || dest->mydest == DestIntoRel)
use_parallel_mode = false;
if (use_parallel_mode)
@@ -1667,7 +1677,11 @@ ExecutePlan(EState *estate,
*/
current_tuple_count++;
if (numberTuples && numberTuples == current_tuple_count)
+ {
+ /* Allow nodes to release or shut down resources. */
+ (void) ExecShutdownNode(planstate);
break;
+ }
}
if (use_parallel_mode)
diff --git a/src/backend/executor/execParallel.c b/src/backend/executor/execParallel.c
index de0e2ba..9ab3334 100644
--- a/src/backend/executor/execParallel.c
+++ b/src/backend/executor/execParallel.c
@@ -838,7 +838,7 @@ ParallelQueryMain(dsm_segment *seg, shm_toc *toc)
ExecParallelInitializeWorker(queryDesc->planstate, toc);
/* Run the plan */
- ExecutorRun(queryDesc, ForwardScanDirection, 0L);
+ ExecutorRun(queryDesc, ForwardScanDirection, 0L, true);
/* Shut down the executor */
ExecutorFinish(queryDesc);
diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index 2d49a65..a5187b0 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -855,7 +855,7 @@ postquel_getnext(execution_state *es, SQLFunctionCachePtr fcache)
/* Run regular commands to completion unless lazyEval */
uint64 count = (es->lazyEval) ? 1 : 0;
- ExecutorRun(es->qd, ForwardScanDirection, count);
+ ExecutorRun(es->qd, ForwardScanDirection, count, !es->lazyEval);
/*
* If we requested run to completion OR there was no tuple returned,
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 55f97b1..72c7b4d 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2305,7 +2305,7 @@ _SPI_pquery(QueryDesc *queryDesc, bool fire_triggers, uint64 tcount)
ExecutorStart(queryDesc, eflags);
- ExecutorRun(queryDesc, ForwardScanDirection, tcount);
+ ExecutorRun(queryDesc, ForwardScanDirection, tcount, true);
_SPI_current->processed = queryDesc->estate->es_processed;
_SPI_current->lastoid = queryDesc->estate->es_lastoid;
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index b07d6c6..707e7c3 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -1101,6 +1101,7 @@ exec_simple_query(const char *query_string)
(void) PortalRun(portal,
FETCH_ALL,
isTopLevel,
+ true,
receiver,
receiver,
completionTag);
@@ -1985,6 +1986,7 @@ exec_execute_message(const char *portal_name, long max_rows)
completed = PortalRun(portal,
max_rows,
true, /* always top level */
+ !execute_is_fetch && max_rows == FETCH_ALL,
receiver,
receiver,
completionTag);
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index 371d735..f538b77 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -90,6 +90,9 @@ CreateQueryDesc(PlannedStmt *plannedstmt,
qd->planstate = NULL;
qd->totaltime = NULL;
+ /* not yet executed */
+ qd->already_executed = false;
+
return qd;
}
@@ -152,7 +155,7 @@ ProcessQuery(PlannedStmt *plan,
/*
* Run the plan to completion.
*/
- ExecutorRun(queryDesc, ForwardScanDirection, 0L);
+ ExecutorRun(queryDesc, ForwardScanDirection, 0L, true);
/*
* Build command completion status string, if caller wants one.
@@ -679,7 +682,7 @@ PortalSetResultFormat(Portal portal, int nFormats, int16 *formats)
* suspended due to exhaustion of the count parameter.
*/
bool
-PortalRun(Portal portal, long count, bool isTopLevel,
+PortalRun(Portal portal, long count, bool isTopLevel, bool run_once,
DestReceiver *dest, DestReceiver *altdest,
char *completionTag)
{
@@ -712,6 +715,10 @@ PortalRun(Portal portal, long count, bool isTopLevel,
*/
MarkPortalActive(portal);
+ /* Set run_once flag. Shouldn't be clear if previously set. */
+ Assert(!portal->run_once || run_once);
+ portal->run_once = run_once;
+
/*
* Set up global portal context pointers.
*
@@ -918,7 +925,8 @@ PortalRunSelect(Portal portal,
else
{
PushActiveSnapshot(queryDesc->snapshot);
- ExecutorRun(queryDesc, direction, (uint64) count);
+ ExecutorRun(queryDesc, direction, (uint64) count,
+ portal->run_once);
nprocessed = queryDesc->estate->es_processed;
PopActiveSnapshot();
}
@@ -957,7 +965,8 @@ PortalRunSelect(Portal portal,
else
{
PushActiveSnapshot(queryDesc->snapshot);
- ExecutorRun(queryDesc, direction, (uint64) count);
+ ExecutorRun(queryDesc, direction, (uint64) count,
+ portal->run_once);
nprocessed = queryDesc->estate->es_processed;
PopActiveSnapshot();
}
@@ -1394,6 +1403,9 @@ PortalRunFetch(Portal portal,
*/
MarkPortalActive(portal);
+ /* If supporting FETCH, portal can't be run-once. */
+ Assert(!portal->run_once);
+
/*
* Set up global portal context pointers.
*/
diff --git a/src/include/executor/execdesc.h b/src/include/executor/execdesc.h
index c99ea81..87e7ca8 100644
--- a/src/include/executor/execdesc.h
+++ b/src/include/executor/execdesc.h
@@ -47,6 +47,9 @@ typedef struct QueryDesc
EState *estate; /* executor's query-wide state */
PlanState *planstate; /* tree of per-plan-node state */
+ /* This field is set by ExecutorRun */
+ bool already_executed; /* true if previously executed */
+
/* This is always set NULL by the core system, but plugins can change it */
struct Instrumentation *totaltime; /* total time spent in ExecutorRun */
} QueryDesc;
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 02dbe7b..7333569 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -81,7 +81,8 @@ extern PGDLLIMPORT ExecutorStart_hook_type ExecutorStart_hook;
/* Hook for plugins to get control in ExecutorRun() */
typedef void (*ExecutorRun_hook_type) (QueryDesc *queryDesc,
ScanDirection direction,
- uint64 count);
+ uint64 count,
+ bool execute_once);
extern PGDLLIMPORT ExecutorRun_hook_type ExecutorRun_hook;
/* Hook for plugins to get control in ExecutorFinish() */
@@ -176,9 +177,9 @@ extern TupleTableSlot *ExecFilterJunk(JunkFilter *junkfilter,
extern void ExecutorStart(QueryDesc *queryDesc, int eflags);
extern void standard_ExecutorStart(QueryDesc *queryDesc, int eflags);
extern void ExecutorRun(QueryDesc *queryDesc,
- ScanDirection direction, uint64 count);
+ ScanDirection direction, uint64 count, bool execute_once);
extern void standard_ExecutorRun(QueryDesc *queryDesc,
- ScanDirection direction, uint64 count);
+ ScanDirection direction, uint64 count, bool execute_once);
extern void ExecutorFinish(QueryDesc *queryDesc);
extern void standard_ExecutorFinish(QueryDesc *queryDesc);
extern void ExecutorEnd(QueryDesc *queryDesc);
diff --git a/src/include/tcop/pquery.h b/src/include/tcop/pquery.h
index 61c0b34..12ff458 100644
--- a/src/include/tcop/pquery.h
+++ b/src/include/tcop/pquery.h
@@ -34,7 +34,7 @@ extern void PortalSetResultFormat(Portal portal, int nFormats,
int16 *formats);
extern bool PortalRun(Portal portal, long count, bool isTopLevel,
- DestReceiver *dest, DestReceiver *altdest,
+ bool run_once, DestReceiver *dest, DestReceiver *altdest,
char *completionTag);
extern uint64 PortalRunFetch(Portal portal,
diff --git a/src/include/utils/portal.h b/src/include/utils/portal.h
index dc76acd..e7c5a8b 100644
--- a/src/include/utils/portal.h
+++ b/src/include/utils/portal.h
@@ -141,6 +141,7 @@ typedef struct PortalData
/* Features/options */
PortalStrategy strategy; /* see above */
int cursorOptions; /* DECLARE CURSOR option bits */
+ bool run_once; /* portal will only be run once */
/* Status data */
PortalStatus status; /* see above */
On Wed, Mar 8, 2017 at 1:54 AM, Robert Haas <robertmhaas@gmail.com> wrote:
Here's a draft patch showing the sort of thing I have in mind. I
think it needs more work, but it gives you the idea, I hope. This is
loosely based on your pl_parallel_exec_support_v1.patch, but what I've
done here is added some flags that carry the information about whether
there will be only one or maybe more than one call to ExecutorRun to a
bunch of relevant places.I think this might have the effect of disabling parallel query in some
cases where PL/pgsql currently allows it, and I think that may be
necessary. (We may need to back-patch a different fix into 9.6.)
I wanted to clarify a few things here, I noticed that call of ExecutorRun
in postquel_getnext() uses !es->lazyEval as execute_once, this is
confusing, as it is true even in cases when a simple query like "select
count(*) from t" is used in a sql function. Hence, restricting parallelism
for cases when it shouldn't. It seems to me that es->lazyEval is not set
properly or it should not be true for simple select statements. I found
that in the definition of execution_state
bool lazyEval; /* true if should fetch one row at a time */
and in init_execution_state, there is a comment saying,
* Mark the last canSetTag query as delivering the function result; then,
* if it is a plain SELECT, mark it for lazy evaluation. If it's not a
* SELECT we must always run it to completion.
I find these two things contradictory to each other. So, is this point
missed or is there some deep reasoning behind that?
--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/
On Wed, Mar 8, 2017 at 1:54 AM, Robert Haas <robertmhaas@gmail.com> wrote:
There are two places where we currently set CURSOR_OPT_PARALLEL_OK in
PL/pgsql: exec_stmt_return_query() sets it when calling
exec_dynquery_with_params(), and exec_run_select() calls it when
calling exec_prepare_plan() if parallelOK is set. The latter is OK,
because exec_run_select() runs the plan via
SPI_execute_plan_with_paramlist(), which calls _SPI_execute_plan(),
which calls _SPI_pquery(). But the former is broken, because
exec_stmt_return_query() executes the query by calling
SPI_cursor_fetch() with a fetch count of 50, and that calls
_SPI_cursor_operation() which calls PortalRunFetch() -- and of course
each call to PortalRunFetch() is going to cause a separate call to
PortalRunSelect(), resulting in a separate call to ExecutorRun().
Oops.
Fixed. The attached patch is over execute_once.patch [1]/messages/by-id/CA+TgmobXEhvHbJtWDuPZM9bVSLiTj-kShxQJ2uM5GPDze9fRYA@mail.gmail.com -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/.
I haven't addressed the issue regarding the confusion I raised upthread
about incorrect value of !es->lazyeval that is restricting parallelism for
simple queries coming from sql functions, as I am not sure if it is the
concern of this patch or not.
[1]: /messages/by-id/CA+TgmobXEhvHbJtWDuPZM9bVSLiTj-kShxQJ2uM5GPDze9fRYA@mail.gmail.com -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/
/messages/by-id/CA+TgmobXEhvHbJtWDuPZM9bVSLiTj-kShxQJ2uM5GPDze9fRYA@mail.gmail.com
--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/
Attachments:
pl_parallel_opt_support_v2.patchapplication/octet-stream; name=pl_parallel_opt_support_v2.patchDownload
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 72c7b4d068..eeaa4805e4 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -314,7 +314,7 @@ SPI_execute(const char *src, bool read_only, long tcount)
memset(&plan, 0, sizeof(_SPI_plan));
plan.magic = _SPI_PLAN_MAGIC;
- plan.cursor_options = 0;
+ plan.cursor_options = CURSOR_OPT_PARALLEL_OK;
_SPI_prepare_oneshot_plan(src, &plan);
@@ -458,7 +458,7 @@ SPI_execute_with_args(const char *src,
memset(&plan, 0, sizeof(_SPI_plan));
plan.magic = _SPI_PLAN_MAGIC;
- plan.cursor_options = 0;
+ plan.cursor_options = CURSOR_OPT_PARALLEL_OK;
plan.nargs = nargs;
plan.argtypes = argtypes;
plan.parserSetup = NULL;
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 196e518e0d..8b638e39ab 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3031,7 +3031,7 @@ exec_stmt_return_query(PLpgSQL_execstate *estate,
Assert(stmt->dynquery != NULL);
portal = exec_dynquery_with_params(estate, stmt->dynquery,
stmt->params, NULL,
- CURSOR_OPT_PARALLEL_OK);
+ 0);
}
/* Use eval_mcontext for tuple conversion work */
@@ -3627,7 +3627,12 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
{
ListCell *l;
- exec_prepare_plan(estate, expr, 0);
+ /* Allow parallelism if the function is not trigger type. */
+ if (estate->func->fn_is_trigger == PLPGSQL_NOT_TRIGGER)
+ exec_prepare_plan(estate, expr, CURSOR_OPT_PARALLEL_OK);
+ else
+ exec_prepare_plan(estate, expr, 0);
+
stmt->mod_stmt = false;
foreach(l, SPI_plan_get_plan_sources(expr->plan))
{
@@ -5174,7 +5179,7 @@ exec_eval_expr(PLpgSQL_execstate *estate,
* If first time through, create a plan for this expression.
*/
if (expr->plan == NULL)
- exec_prepare_plan(estate, expr, 0);
+ exec_prepare_plan(estate, expr, CURSOR_OPT_PARALLEL_OK);
/*
* If this is a simple expression, bypass SPI and use the executor
@@ -5185,9 +5190,14 @@ exec_eval_expr(PLpgSQL_execstate *estate,
return result;
/*
- * Else do it the hard way via exec_run_select
+ * Else do it the hard way via exec_run_select. Enable parallelism if
+ * function is not trigger type.
*/
- rc = exec_run_select(estate, expr, 2, NULL, false);
+ if (estate->func->fn_is_trigger == PLPGSQL_NOT_TRIGGER)
+ rc = exec_run_select(estate, expr, 2, NULL, true);
+ else
+ rc = exec_run_select(estate, expr, 2, NULL, false);
+
if (rc != SPI_OK_SELECT)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
On Mon, Mar 13, 2017 at 5:48 PM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:
Fixed. The attached patch is over execute_once.patch [1].
I haven't addressed the issue regarding the confusion I raised upthread
about incorrect value of !es->lazyeval that is restricting parallelism for
simple queries coming from sql functions, as I am not sure if it is the
concern of this patch or not.
I have reviewed the patch, I have some questions.
@@ -3031,7 +3031,7 @@ exec_stmt_return_query(PLpgSQL_execstate *estate,
Assert(stmt->dynquery != NULL);
portal = exec_dynquery_with_params(estate, stmt->dynquery,
stmt->params, NULL,
- CURSOR_OPT_PARALLEL_OK);
+ 0);After this patch, I have noticed that In exec_stmt_return_query, we
allow parallel query if it's a static query
but not for the dynamic query. Any specific reason for different
handling for these 2 cases?
@ -314,7 +314,7 @@ SPI_execute(const char *src, bool read_only, long tcount)
memset(&plan, 0, sizeof(_SPI_plan));
plan.magic = _SPI_PLAN_MAGIC;
- plan.cursor_options = 0;
+ plan.cursor_options = CURSOR_OPT_PARALLEL_OK;In SPI_Execute, we are setting cursor_options to
CURSOR_OPT_PARALLEL_OK. I have analysed call to this function from PL
and seems fine to me. But, I have a question have you analyzed all the
calls to this functions?
e.g. build_tuplestore_recursively, get_crosstab_tuplestore.
On Fri, Mar 10, 2017 at 5:38 PM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:
I wanted to clarify a few things here, I noticed that call of ExecutorRun in
postquel_getnext() uses !es->lazyEval as execute_once, this is confusing, as
it is true even in cases when a simple query like "select count(*) from t"
is used in a sql function. Hence, restricting parallelism for cases when it
shouldn't. It seems to me that es->lazyEval is not set properly or it should
not be true for simple select statements. I found that in the definition of
execution_state
bool lazyEval; /* true if should fetch one row at a time
Hmm, It seems that es->lazyEval is not set properly, Ideally, it
should be set true only if it's lazy evaluation but in this case, it's
used for identifying the tupcount as well. IMHO, it should be fixed.
Any other opinion on this?
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Mar 15, 2017 at 10:52 AM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
I have reviewed the patch, I have some questions.
@@ -3031,7 +3031,7 @@ exec_stmt_return_query(PLpgSQL_execstate *estate, Assert(stmt->dynquery != NULL); portal = exec_dynquery_with_params(estate, stmt->dynquery, stmt->params, NULL, - CURSOR_OPT_PARALLEL_OK); + 0);After this patch, I have noticed that In exec_stmt_return_query, we
allow parallel query if it's a static query
but not for the dynamic query. Any specific reason for different
handling for these 2 cases?
The reason for such behaviour is given upthread, in
exec_stmt_return_query, the query may be executed by either
exec_run_select which then uses SPI_execute_plan_with_paramlist(),
which calls _SPI_execute_plan(), which calls _SPI_pquery(), hence if
parallelOK is set then passing CURSOR_OPT_PARALLEL_OK is safe.
Otherwise, exec_stmt_return_query executes with a call to
SPI_cursor_fetch() with a fetch count of 50, and that calls
_SPI_cursor_operation() which calls PortalRunFetch(), hence, passing
CURSOR_OPT_PARALLEL_OK would not be safe in this case hence not
passed.
@ -314,7 +314,7 @@ SPI_execute(const char *src, bool read_only, long tcount)
memset(&plan, 0, sizeof(_SPI_plan)); plan.magic = _SPI_PLAN_MAGIC; - plan.cursor_options = 0; + plan.cursor_options = CURSOR_OPT_PARALLEL_OK;In SPI_Execute, we are setting cursor_options to
CURSOR_OPT_PARALLEL_OK. I have analysed call to this function from PL
and seems fine to me. But, I have a question have you analyzed all the
calls to this functions?
e.g. build_tuplestore_recursively, get_crosstab_tuplestore.
The thing with SPI_execute is that it calls pg_plan_query through
_SPI_execute_plan, there if the query is parallel unsafe then
parallelism will be restricted by planner itself otherwise it is
enabled if CURSOR_OPT_PARALLEL_OK was set. So, yes I evaluated all the
calls for this functions and here is the analysis of why it should be
safe passing CURSOR_OPT_PARALLEL_OK, in some of the suspicious looking
calls(other calls are directly related to spy functions)
In crosstab: ret = SPI_execute(sql, true, 0);
In load_categories_hash: ret = SPI_execute(cats_sql, true, 0);
In get_crosstab_tuplestore: ret = SPI_execute(sql, true, 0);
In build_tuplestore_recursively: ret = SPI_execute(sql.data, true, 0);
In query_to_oid_list: SPI_execute(query, true, 0);
In all of these calls, read_only flag is passed to be true, hence
enabling parallelism will not cause any anomalous behaviour.
In refresh_by_match_merge: if (SPI_execute(querybuf.data, false, 1) !=
SPI_OK_SELECT)
In this case, since read_only is set to false, hence, in SPI_execute
when planner will recalled for such a case it will disable
parallelism, hence, no issues.
if ((cursorOptions & CURSOR_OPT_PARALLEL_OK) != 0 &&
IsUnderPostmaster &&
dynamic_shared_memory_type != DSM_IMPL_NONE &&
parse->commandType == CMD_SELECT &&
!parse->hasModifyingCTE &&
max_parallel_workers_per_gather > 0 &&
!IsParallelWorker() &&
!IsolationIsSerializable())
On Fri, Mar 10, 2017 at 5:38 PM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:I wanted to clarify a few things here, I noticed that call of ExecutorRun in
postquel_getnext() uses !es->lazyEval as execute_once, this is confusing, as
it is true even in cases when a simple query like "select count(*) from t"
is used in a sql function. Hence, restricting parallelism for cases when it
shouldn't. It seems to me that es->lazyEval is not set properly or it should
not be true for simple select statements. I found that in the definition of
execution_state
bool lazyEval; /* true if should fetch one row at a timeHmm, It seems that es->lazyEval is not set properly, Ideally, it
should be set true only if it's lazy evaluation but in this case, it's
used for identifying the tupcount as well. IMHO, it should be fixed.Any other opinion on this?
Hmmm... I tried investigating into this but it looks like there isn't
much scope for this. LazyEvalOk is set for SELECT commands in
init_execution_state as per,
/*
* Mark the last canSetTag query as delivering the function result; then,
* if it is a plain SELECT, mark it for lazy evaluation. If it's not a
* SELECT we must always run it to completion.
...
if (lasttages && fcache->junkFilter)
{
lasttages->setsResult = true;
if (lazyEvalOK &&
lasttages->stmt->commandType == CMD_SELECT &&
!lasttages->stmt->hasModifyingCTE)
fcache->lazyEval = lasttages->lazyEval = true;
}
and then in postquel_getnext we set execute_once = !es->lazyEval [1]/messages/by-id/CA+TgmobXEhvHbJtWDuPZM9bVSLiTj-kShxQJ2uM5GPDze9fRYA@mail.gmail.com -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/,
which also makes sense since,
/* Run regular commands to completion unless lazyEval */
Hence, this situation looks like it is restricting parallelism for
some cases where it might not cause any issues, but clearly modifying
lazyEval is not a safe option. Additionally, I think we do not have
enough information to ensure that a select query will not cause
multiple ExecutorRun calls for these user-defined queries inside SQL
functions. Moreover, lazyEval is a kind of may be thing, meaning that
it might call ExecutorRun multiple times when set but not ensured that
it will as in the case of select count(*) from t queries.
Please let me know your views on this.
[1]: /messages/by-id/CA+TgmobXEhvHbJtWDuPZM9bVSLiTj-kShxQJ2uM5GPDze9fRYA@mail.gmail.com -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/
--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Mar 10, 2017 at 7:08 AM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:
I wanted to clarify a few things here, I noticed that call of ExecutorRun in
postquel_getnext() uses !es->lazyEval as execute_once, this is confusing, as
it is true even in cases when a simple query like "select count(*) from t"
is used in a sql function. Hence, restricting parallelism for cases when it
shouldn't. It seems to me that es->lazyEval is not set properly or it should
not be true for simple select statements. I found that in the definition of
execution_state
bool lazyEval; /* true if should fetch one row at a time */
and in init_execution_state, there is a comment saying,
* Mark the last canSetTag query as delivering the function result; then,
* if it is a plain SELECT, mark it for lazy evaluation. If it's not a
* SELECT we must always run it to completion.I find these two things contradictory to each other. So, is this point
missed or is there some deep reasoning behind that?
I don't understand what you think is contradictory. I think the idea
is that if it's not a SELECT, we have to run it to completion because
it might have side effects, but if it is a SELECT, we assume (granted,
it might be wrong) that there are no side effects, and therefore we
can just run it until it produces the number of rows of output that we
need.
Note this:
if (completed || !fcache->returnsSet)
postquel_end(es);
When the SQL function doesn't return a set, then we can allow
parallelism even when lazyEval is set, because we'll only call
ExecutorStart() once. But my impression is that something like this:
SELECT * FROM blah() LIMIT 3
...will trigger three separate calls to ExecutorRun(), which is a
problem if the plan is a parallel plan.
I have not verified this; the above thoughts are just based on code-reading.
--
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 Wed, Mar 15, 2017 at 8:55 PM, Robert Haas <robertmhaas@gmail.com> wrote:
Note this:
if (completed || !fcache->returnsSet)
postquel_end(es);When the SQL function doesn't return a set, then we can allow
parallelism even when lazyEval is set, because we'll only call
ExecutorStart() once. But my impression is that something like this:
Well, when I test following SQL function I see it cannot be
parallelised because lazyEval is true for it though it is not
returning set,
CREATE OR REPLACE FUNCTION not_parallel()
RETURNS bigint AS $$
BEGIN
SELECT count(distinct i) FROM t WHERE j = 12;
END;
$$ LANGUAGE sql;
Query Text:
SELECT count(distinct i) FROM t WHERE j = 12;
Aggregate (cost=34.02..34.02 rows=1 width=8) (actual
time=0.523..0.523 rows=1 loops=1)
-> Seq Scan on t (cost=0.00..34.01 rows=1 width=4) (actual
time=0.493..0.493 rows=0 loops=1)
Filter: (j = 12)
Rows Removed by Filter: 2001
2017-03-21 15:24:03.378 IST [117823] CONTEXT: SQL function
"already_parallel" statement 1
2017-03-21 15:24:03.378 IST [117823] LOG: duration: 94868.181 ms plan:
Query Text: select already_parallel();
Result (cost=0.00..0.26 rows=1 width=8) (actual
time=87981.047..87981.048 rows=1 loops=1)
already_parallel
------------------
0
(1 row)
As far as my understanding goes for this case, lazyEvalOk is set
irrespective of whether the function returns set or not in fmgr_sql,
else
{
randomAccess = false;
lazyEvalOK = true;
}
then it is passed to init_sql_fcache which is then passed to
init_execution_state where cache->lazyEval is set,
if (lasttages && fcache->junkFilter)
{
lasttages->setsResult = true;
if (lazyEvalOK &&
lasttages->stmt->commandType == CMD_SELECT &&
!lasttages->stmt->hasModifyingCTE)
fcache->lazyEval = lasttages->lazyEval = true;
}
Finally, this lazyEval is passed to ExecutorRun in postquel_getnext
that restricts parallelism by setting execute_once = 0,
/* Run regular commands to completion unless lazyEval */
uint64 count = (es->lazyEval) ? 1 : 0;
ExecutorRun(es->qd, ForwardScanDirection, count, !es->lazyEval);
So, this is my concern that why is such a query should not execute in
parallel when in SQL function. If I run this same query from PLpgsql
function then it can run in parallel,
CREATE OR REPLACE FUNCTION not_parallel()
RETURNS bigint AS $$
declare cnt int:=0;
BEGIN
SELECT count(distinct i) into cnt FROM t WHERE j = 12;
RETURN cnt;
END;
$$ LANGUAGE plpgsql;
select not_parallel();
2017-03-21 15:28:56.282 IST [123086] LOG: duration: 0.003 ms plan:
Query Text: SELECT count(distinct i) FROM t WHERE j = 12
Parallel Seq Scan on t (cost=0.00..19.42 rows=1 width=4) (actual
time=0.001..0.001 rows=0 loops=1)
Filter: (j = 12)
2017-03-21 15:28:56.282 IST [123087] LOG: duration: 0.003 ms plan:
Query Text: SELECT count(distinct i) FROM t WHERE j = 12
Parallel Seq Scan on t (cost=0.00..19.42 rows=1 width=4) (actual
time=0.001..0.001 rows=0 loops=1)
Filter: (j = 12)
2017-03-21 15:28:57.530 IST [117823] LOG: duration: 1745.372 ms plan:
Query Text: SELECT count(distinct i) FROM t WHERE j = 12
Aggregate (cost=19.42..19.43 rows=1 width=8) (actual
time=1255.743..1255.743 rows=1 loops=1)
-> Gather (cost=0.00..19.42 rows=1 width=4) (actual
time=1255.700..1255.700 rows=0 loops=1)
Workers Planned: 2
Workers Launched: 2
-> Parallel Seq Scan on t (cost=0.00..19.42 rows=1 width=4)
(actual time=418.443..418.443 rows=0 loops=3)
Filter: (j = 12)
Rows Removed by Filter: 667
2017-03-21 15:28:57.530 IST [117823] CONTEXT: SQL statement "SELECT
count(distinct i) FROM t WHERE j = 12"
PL/pgSQL function not_parallel() line 4 at SQL statement
2017-03-21 15:28:57.531 IST [117823] LOG: duration: 2584.282 ms plan:
Query Text: select not_parallel();
Result (cost=0.00..0.26 rows=1 width=8) (actual
time=2144.315..2144.316 rows=1 loops=1)
not_parallel
--------------
0
(1 row)
Hence, it appears lazyEval is the main reason behind it and it should
be definitely fixed in my opinion.
Please enlighten me with your comments/opinions.
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/
--
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 21, 2017 at 3:36 PM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:
On Wed, Mar 15, 2017 at 8:55 PM, Robert Haas <robertmhaas@gmail.com> wrote:
Note this:
if (completed || !fcache->returnsSet)
postquel_end(es);When the SQL function doesn't return a set, then we can allow
parallelism even when lazyEval is set, because we'll only call
ExecutorStart() once. But my impression is that something like this:
How about taking the decision of execute_once based on
fcache->returnsSet instead of based on lazyEval?
change
+ ExecutorRun(es->qd, ForwardScanDirection, count, !es->lazyEval);
to
+ ExecutorRun(es->qd, ForwardScanDirection, count, !fcache->returnsSet);IMHO, Robert have the same thing in mind?
SELECT * FROM blah() LIMIT 3
...will trigger three separate calls to ExecutorRun(), which is a
problem if the plan is a parallel plan.
And you also need to test this case what Robert have mentioned up thread.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
--
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 21, 2017 at 6:36 AM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Tue, Mar 21, 2017 at 3:36 PM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:On Wed, Mar 15, 2017 at 8:55 PM, Robert Haas <robertmhaas@gmail.com> wrote:
Note this:
if (completed || !fcache->returnsSet)
postquel_end(es);When the SQL function doesn't return a set, then we can allow
parallelism even when lazyEval is set, because we'll only call
ExecutorStart() once. But my impression is that something like this:How about taking the decision of execute_once based on
fcache->returnsSet instead of based on lazyEval?change + ExecutorRun(es->qd, ForwardScanDirection, count, !es->lazyEval); to + ExecutorRun(es->qd, ForwardScanDirection, count, !fcache->returnsSet);IMHO, Robert have the same thing in mind?
Yeah, something like that.
SELECT * FROM blah() LIMIT 3
...will trigger three separate calls to ExecutorRun(), which is a
problem if the plan is a parallel plan.And you also need to test this case what Robert have mentioned up thread.
+1
--
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 Wed, Mar 22, 2017 at 12:55 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Mar 21, 2017 at 6:36 AM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
How about taking the decision of execute_once based on
fcache->returnsSet instead of based on lazyEval?change + ExecutorRun(es->qd, ForwardScanDirection, count, !es->lazyEval); to + ExecutorRun(es->qd, ForwardScanDirection, count, !fcache->returnsSet);IMHO, Robert have the same thing in mind?
Yeah, something like that.
Done in execute-once-v2.patch
Apart from this, I also observed that in case of SQL functions,
cursorOptions are not set properly, note in init_execution_state,
stmt = pg_plan_query(queryTree,
fcache->readonly_func ? CURSOR_OPT_PARALLEL_OK : 0,
NULL)
Now, this fcache->readonly_func is set in init_sql_fcache,
/* Remember if function is STABLE/IMMUTABLE */
fcache->readonly_func =
(procedureStruct->provolatile != PROVOLATILE_VOLATILE);
so, if parallel safe stable is missing in function definition then it
is not as readonly as per this code. Also, we can see that this is set
as per function rather than per query as in case of other PL
functions. So, I did
stmt = pg_plan_query(queryTree,
- fcache->readonly_func ? CURSOR_OPT_PARALLEL_OK : 0,
+ CURSOR_OPT_PARALLEL_OK,
NULL);Now, if the query is an update/insert/delete statement then it is
anyways taken care by planner and is not parallelised. This also
enables parallelism for the case when one query is select and another
is update in an SQL function which couldn't be done before.
This is done in pl_parallel_opt_v3.patch.
SELECT * FROM blah() LIMIT 3
...will trigger three separate calls to ExecutorRun(), which is a
problem if the plan is a parallel plan.And you also need to test this case what Robert have mentioned up thread.
+1
Checked, nope ExecutorRun is called only once in this case and
execute_once is true here.
--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/
Attachments:
pl_parallel_opt_support_v3.patchapplication/octet-stream; name=pl_parallel_opt_support_v3.patchDownload
diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index 2d49a65650..5ce6a3ddca 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -503,7 +503,7 @@ init_execution_state(List *queryTree_list,
}
else
stmt = pg_plan_query(queryTree,
- fcache->readonly_func ? CURSOR_OPT_PARALLEL_OK : 0,
+ CURSOR_OPT_PARALLEL_OK,
NULL);
/*
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 55f97b14e6..1f6e669327 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -314,7 +314,7 @@ SPI_execute(const char *src, bool read_only, long tcount)
memset(&plan, 0, sizeof(_SPI_plan));
plan.magic = _SPI_PLAN_MAGIC;
- plan.cursor_options = 0;
+ plan.cursor_options = CURSOR_OPT_PARALLEL_OK;
_SPI_prepare_oneshot_plan(src, &plan);
@@ -458,7 +458,7 @@ SPI_execute_with_args(const char *src,
memset(&plan, 0, sizeof(_SPI_plan));
plan.magic = _SPI_PLAN_MAGIC;
- plan.cursor_options = 0;
+ plan.cursor_options = CURSOR_OPT_PARALLEL_OK;
plan.nargs = nargs;
plan.argtypes = argtypes;
plan.parserSetup = NULL;
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 49a4e622ff..86b8c41415 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3031,7 +3031,7 @@ exec_stmt_return_query(PLpgSQL_execstate *estate,
Assert(stmt->dynquery != NULL);
portal = exec_dynquery_with_params(estate, stmt->dynquery,
stmt->params, NULL,
- CURSOR_OPT_PARALLEL_OK);
+ 0);
}
/* Use eval_mcontext for tuple conversion work */
@@ -3627,7 +3627,12 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
{
ListCell *l;
- exec_prepare_plan(estate, expr, 0);
+ /* Allow parallelism if the function is not trigger type. */
+ if (estate->func->fn_is_trigger == PLPGSQL_NOT_TRIGGER)
+ exec_prepare_plan(estate, expr, CURSOR_OPT_PARALLEL_OK);
+ else
+ exec_prepare_plan(estate, expr, 0);
+
stmt->mod_stmt = false;
foreach(l, SPI_plan_get_plan_sources(expr->plan))
{
@@ -5174,7 +5179,7 @@ exec_eval_expr(PLpgSQL_execstate *estate,
* If first time through, create a plan for this expression.
*/
if (expr->plan == NULL)
- exec_prepare_plan(estate, expr, 0);
+ exec_prepare_plan(estate, expr, CURSOR_OPT_PARALLEL_OK);
/*
* If this is a simple expression, bypass SPI and use the executor
@@ -5185,9 +5190,14 @@ exec_eval_expr(PLpgSQL_execstate *estate,
return result;
/*
- * Else do it the hard way via exec_run_select
+ * Else do it the hard way via exec_run_select. Enable parallelism if
+ * function is not trigger type.
*/
- rc = exec_run_select(estate, expr, 2, NULL, false);
+ if (estate->func->fn_is_trigger == PLPGSQL_NOT_TRIGGER)
+ rc = exec_run_select(estate, expr, 2, NULL, true);
+ else
+ rc = exec_run_select(estate, expr, 2, NULL, false);
+
if (rc != SPI_OK_SELECT)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
execute-once-v2.patchapplication/octet-stream; name=execute-once-v2.patchDownload
diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c
index 34b9f1543e..9213ffb6a4 100644
--- a/contrib/auto_explain/auto_explain.c
+++ b/contrib/auto_explain/auto_explain.c
@@ -61,7 +61,7 @@ void _PG_fini(void);
static void explain_ExecutorStart(QueryDesc *queryDesc, int eflags);
static void explain_ExecutorRun(QueryDesc *queryDesc,
ScanDirection direction,
- uint64 count);
+ uint64 count, bool execute_once);
static void explain_ExecutorFinish(QueryDesc *queryDesc);
static void explain_ExecutorEnd(QueryDesc *queryDesc);
@@ -257,15 +257,16 @@ explain_ExecutorStart(QueryDesc *queryDesc, int eflags)
* ExecutorRun hook: all we need do is track nesting depth
*/
static void
-explain_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, uint64 count)
+explain_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction,
+ uint64 count, bool execute_once)
{
nesting_level++;
PG_TRY();
{
if (prev_ExecutorRun)
- prev_ExecutorRun(queryDesc, direction, count);
+ prev_ExecutorRun(queryDesc, direction, count, execute_once);
else
- standard_ExecutorRun(queryDesc, direction, count);
+ standard_ExecutorRun(queryDesc, direction, count, execute_once);
nesting_level--;
}
PG_CATCH();
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 221ac98d4a..42f43233f8 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -290,7 +290,7 @@ static void pgss_post_parse_analyze(ParseState *pstate, Query *query);
static void pgss_ExecutorStart(QueryDesc *queryDesc, int eflags);
static void pgss_ExecutorRun(QueryDesc *queryDesc,
ScanDirection direction,
- uint64 count);
+ uint64 count, bool execute_once);
static void pgss_ExecutorFinish(QueryDesc *queryDesc);
static void pgss_ExecutorEnd(QueryDesc *queryDesc);
static void pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
@@ -871,15 +871,16 @@ pgss_ExecutorStart(QueryDesc *queryDesc, int eflags)
* ExecutorRun hook: all we need do is track nesting depth
*/
static void
-pgss_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, uint64 count)
+pgss_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, uint64 count,
+ bool execute_once)
{
nested_level++;
PG_TRY();
{
if (prev_ExecutorRun)
- prev_ExecutorRun(queryDesc, direction, count);
+ prev_ExecutorRun(queryDesc, direction, count, execute_once);
else
- standard_ExecutorRun(queryDesc, direction, count);
+ standard_ExecutorRun(queryDesc, direction, count, execute_once);
nested_level--;
}
PG_CATCH();
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index ba89b292d1..54aba9156a 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2067,7 +2067,7 @@ CopyTo(CopyState cstate)
else
{
/* run the plan --- the dest receiver will send tuples */
- ExecutorRun(cstate->queryDesc, ForwardScanDirection, 0L);
+ ExecutorRun(cstate->queryDesc, ForwardScanDirection, 0L, true);
processed = ((DR_copy *) cstate->queryDesc->dest)->processed;
}
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 646a88409f..3daffc894a 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -347,7 +347,7 @@ ExecCreateTableAs(CreateTableAsStmt *stmt, const char *queryString,
ExecutorStart(queryDesc, GetIntoRelEFlags(into));
/* run the plan to completion */
- ExecutorRun(queryDesc, ForwardScanDirection, 0L);
+ ExecutorRun(queryDesc, ForwardScanDirection, 0L, true);
/* save the rowcount if we're given a completionTag to fill */
if (completionTag)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index c9b55ead3d..b4c7466666 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -530,7 +530,7 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
dir = ForwardScanDirection;
/* run the plan */
- ExecutorRun(queryDesc, dir, 0L);
+ ExecutorRun(queryDesc, dir, 0L, true);
/* run cleanup too */
ExecutorFinish(queryDesc);
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 86a84ee234..5a84bedf46 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -742,7 +742,7 @@ execute_sql_string(const char *sql, const char *filename)
dest, NULL, 0);
ExecutorStart(qdesc, 0);
- ExecutorRun(qdesc, ForwardScanDirection, 0);
+ ExecutorRun(qdesc, ForwardScanDirection, 0, true);
ExecutorFinish(qdesc);
ExecutorEnd(qdesc);
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index a18c917336..ad39df2024 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -404,7 +404,7 @@ refresh_matview_datafill(DestReceiver *dest, Query *query,
ExecutorStart(queryDesc, EXEC_FLAG_WITHOUT_OIDS);
/* run the plan */
- ExecutorRun(queryDesc, ForwardScanDirection, 0L);
+ ExecutorRun(queryDesc, ForwardScanDirection, 0L, true);
/* and clean up */
ExecutorFinish(queryDesc);
diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c
index 29d0430dd8..f57cf87e8c 100644
--- a/src/backend/commands/portalcmds.c
+++ b/src/backend/commands/portalcmds.c
@@ -395,7 +395,7 @@ PersistHoldablePortal(Portal portal)
true);
/* Fetch the result set into the tuplestore */
- ExecutorRun(queryDesc, ForwardScanDirection, 0L);
+ ExecutorRun(queryDesc, ForwardScanDirection, 0L, false);
(*queryDesc->dest->rDestroy) (queryDesc->dest);
queryDesc->dest = NULL;
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 1cf0d2b971..992ba1c9a2 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -301,7 +301,7 @@ ExecuteQuery(ExecuteStmt *stmt, IntoClause *intoClause,
*/
PortalStart(portal, paramLI, eflags, GetActiveSnapshot());
- (void) PortalRun(portal, count, false, dest, dest, completionTag);
+ (void) PortalRun(portal, count, false, true, dest, dest, completionTag);
PortalDrop(portal, false);
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index f5cd65d8a0..4f5b7b3bca 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -85,7 +85,8 @@ static void ExecutePlan(EState *estate, PlanState *planstate,
bool sendTuples,
uint64 numberTuples,
ScanDirection direction,
- DestReceiver *dest);
+ DestReceiver *dest,
+ bool execute_once);
static bool ExecCheckRTEPerms(RangeTblEntry *rte);
static bool ExecCheckRTEPermsModified(Oid relOid, Oid userid,
Bitmapset *modifiedCols,
@@ -288,17 +289,18 @@ standard_ExecutorStart(QueryDesc *queryDesc, int eflags)
*/
void
ExecutorRun(QueryDesc *queryDesc,
- ScanDirection direction, uint64 count)
+ ScanDirection direction, uint64 count,
+ bool execute_once)
{
if (ExecutorRun_hook)
- (*ExecutorRun_hook) (queryDesc, direction, count);
+ (*ExecutorRun_hook) (queryDesc, direction, count, execute_once);
else
- standard_ExecutorRun(queryDesc, direction, count);
+ standard_ExecutorRun(queryDesc, direction, count, execute_once);
}
void
standard_ExecutorRun(QueryDesc *queryDesc,
- ScanDirection direction, uint64 count)
+ ScanDirection direction, uint64 count, bool execute_once)
{
EState *estate;
CmdType operation;
@@ -345,6 +347,11 @@ standard_ExecutorRun(QueryDesc *queryDesc,
* run plan
*/
if (!ScanDirectionIsNoMovement(direction))
+ {
+ if (execute_once && queryDesc->already_executed)
+ elog(ERROR, "can't re-execute query flagged for single execution");
+ queryDesc->already_executed = true;
+
ExecutePlan(estate,
queryDesc->planstate,
queryDesc->plannedstmt->parallelModeNeeded,
@@ -352,7 +359,9 @@ standard_ExecutorRun(QueryDesc *queryDesc,
sendTuples,
count,
direction,
- dest);
+ dest,
+ execute_once);
+ }
/*
* shutdown tuple receiver, if we started it
@@ -1575,7 +1584,8 @@ ExecutePlan(EState *estate,
bool sendTuples,
uint64 numberTuples,
ScanDirection direction,
- DestReceiver *dest)
+ DestReceiver *dest,
+ bool execute_once)
{
TupleTableSlot *slot;
uint64 current_tuple_count;
@@ -1591,12 +1601,12 @@ ExecutePlan(EState *estate,
estate->es_direction = direction;
/*
- * If a tuple count was supplied, we must force the plan to run without
- * parallelism, because we might exit early. Also disable parallelism
- * when writing into a relation, because no database changes are allowed
- * in parallel mode.
+ * If the plan might potentially be executed multiple times, we must force
+ * it to run without parallelism, because we might exit early. Also
+ * disable parallelism when writing into a relation, because no database
+ * changes are allowed in parallel mode.
*/
- if (numberTuples || dest->mydest == DestIntoRel)
+ if (!execute_once || dest->mydest == DestIntoRel)
use_parallel_mode = false;
if (use_parallel_mode)
@@ -1667,7 +1677,11 @@ ExecutePlan(EState *estate,
*/
current_tuple_count++;
if (numberTuples && numberTuples == current_tuple_count)
+ {
+ /* Allow nodes to release or shut down resources. */
+ (void) ExecShutdownNode(planstate);
break;
+ }
}
if (use_parallel_mode)
diff --git a/src/backend/executor/execParallel.c b/src/backend/executor/execParallel.c
index a1289e5f12..948a9194d5 100644
--- a/src/backend/executor/execParallel.c
+++ b/src/backend/executor/execParallel.c
@@ -852,7 +852,7 @@ ParallelQueryMain(dsm_segment *seg, shm_toc *toc)
ExecParallelInitializeWorker(queryDesc->planstate, toc);
/* Run the plan */
- ExecutorRun(queryDesc, ForwardScanDirection, 0L);
+ ExecutorRun(queryDesc, ForwardScanDirection, 0L, true);
/* Shut down the executor */
ExecutorFinish(queryDesc);
diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index 2d49a65650..98963a367a 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -855,7 +855,7 @@ postquel_getnext(execution_state *es, SQLFunctionCachePtr fcache)
/* Run regular commands to completion unless lazyEval */
uint64 count = (es->lazyEval) ? 1 : 0;
- ExecutorRun(es->qd, ForwardScanDirection, count);
+ ExecutorRun(es->qd, ForwardScanDirection, count, !fcache->returnsSet);
/*
* If we requested run to completion OR there was no tuple returned,
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 55f97b14e6..72c7b4d068 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2305,7 +2305,7 @@ _SPI_pquery(QueryDesc *queryDesc, bool fire_triggers, uint64 tcount)
ExecutorStart(queryDesc, eflags);
- ExecutorRun(queryDesc, ForwardScanDirection, tcount);
+ ExecutorRun(queryDesc, ForwardScanDirection, tcount, true);
_SPI_current->processed = queryDesc->estate->es_processed;
_SPI_current->lastoid = queryDesc->estate->es_lastoid;
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index b07d6c6cb9..707e7c385d 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -1101,6 +1101,7 @@ exec_simple_query(const char *query_string)
(void) PortalRun(portal,
FETCH_ALL,
isTopLevel,
+ true,
receiver,
receiver,
completionTag);
@@ -1985,6 +1986,7 @@ exec_execute_message(const char *portal_name, long max_rows)
completed = PortalRun(portal,
max_rows,
true, /* always top level */
+ !execute_is_fetch && max_rows == FETCH_ALL,
receiver,
receiver,
completionTag);
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index 371d7350b7..f538b7787c 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -90,6 +90,9 @@ CreateQueryDesc(PlannedStmt *plannedstmt,
qd->planstate = NULL;
qd->totaltime = NULL;
+ /* not yet executed */
+ qd->already_executed = false;
+
return qd;
}
@@ -152,7 +155,7 @@ ProcessQuery(PlannedStmt *plan,
/*
* Run the plan to completion.
*/
- ExecutorRun(queryDesc, ForwardScanDirection, 0L);
+ ExecutorRun(queryDesc, ForwardScanDirection, 0L, true);
/*
* Build command completion status string, if caller wants one.
@@ -679,7 +682,7 @@ PortalSetResultFormat(Portal portal, int nFormats, int16 *formats)
* suspended due to exhaustion of the count parameter.
*/
bool
-PortalRun(Portal portal, long count, bool isTopLevel,
+PortalRun(Portal portal, long count, bool isTopLevel, bool run_once,
DestReceiver *dest, DestReceiver *altdest,
char *completionTag)
{
@@ -712,6 +715,10 @@ PortalRun(Portal portal, long count, bool isTopLevel,
*/
MarkPortalActive(portal);
+ /* Set run_once flag. Shouldn't be clear if previously set. */
+ Assert(!portal->run_once || run_once);
+ portal->run_once = run_once;
+
/*
* Set up global portal context pointers.
*
@@ -918,7 +925,8 @@ PortalRunSelect(Portal portal,
else
{
PushActiveSnapshot(queryDesc->snapshot);
- ExecutorRun(queryDesc, direction, (uint64) count);
+ ExecutorRun(queryDesc, direction, (uint64) count,
+ portal->run_once);
nprocessed = queryDesc->estate->es_processed;
PopActiveSnapshot();
}
@@ -957,7 +965,8 @@ PortalRunSelect(Portal portal,
else
{
PushActiveSnapshot(queryDesc->snapshot);
- ExecutorRun(queryDesc, direction, (uint64) count);
+ ExecutorRun(queryDesc, direction, (uint64) count,
+ portal->run_once);
nprocessed = queryDesc->estate->es_processed;
PopActiveSnapshot();
}
@@ -1394,6 +1403,9 @@ PortalRunFetch(Portal portal,
*/
MarkPortalActive(portal);
+ /* If supporting FETCH, portal can't be run-once. */
+ Assert(!portal->run_once);
+
/*
* Set up global portal context pointers.
*/
diff --git a/src/include/executor/execdesc.h b/src/include/executor/execdesc.h
index c99ea81815..87e7ca8508 100644
--- a/src/include/executor/execdesc.h
+++ b/src/include/executor/execdesc.h
@@ -47,6 +47,9 @@ typedef struct QueryDesc
EState *estate; /* executor's query-wide state */
PlanState *planstate; /* tree of per-plan-node state */
+ /* This field is set by ExecutorRun */
+ bool already_executed; /* true if previously executed */
+
/* This is always set NULL by the core system, but plugins can change it */
struct Instrumentation *totaltime; /* total time spent in ExecutorRun */
} QueryDesc;
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 02dbe7b228..7333569552 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -81,7 +81,8 @@ extern PGDLLIMPORT ExecutorStart_hook_type ExecutorStart_hook;
/* Hook for plugins to get control in ExecutorRun() */
typedef void (*ExecutorRun_hook_type) (QueryDesc *queryDesc,
ScanDirection direction,
- uint64 count);
+ uint64 count,
+ bool execute_once);
extern PGDLLIMPORT ExecutorRun_hook_type ExecutorRun_hook;
/* Hook for plugins to get control in ExecutorFinish() */
@@ -176,9 +177,9 @@ extern TupleTableSlot *ExecFilterJunk(JunkFilter *junkfilter,
extern void ExecutorStart(QueryDesc *queryDesc, int eflags);
extern void standard_ExecutorStart(QueryDesc *queryDesc, int eflags);
extern void ExecutorRun(QueryDesc *queryDesc,
- ScanDirection direction, uint64 count);
+ ScanDirection direction, uint64 count, bool execute_once);
extern void standard_ExecutorRun(QueryDesc *queryDesc,
- ScanDirection direction, uint64 count);
+ ScanDirection direction, uint64 count, bool execute_once);
extern void ExecutorFinish(QueryDesc *queryDesc);
extern void standard_ExecutorFinish(QueryDesc *queryDesc);
extern void ExecutorEnd(QueryDesc *queryDesc);
diff --git a/src/include/tcop/pquery.h b/src/include/tcop/pquery.h
index 61c0b3447e..12ff4588c6 100644
--- a/src/include/tcop/pquery.h
+++ b/src/include/tcop/pquery.h
@@ -34,7 +34,7 @@ extern void PortalSetResultFormat(Portal portal, int nFormats,
int16 *formats);
extern bool PortalRun(Portal portal, long count, bool isTopLevel,
- DestReceiver *dest, DestReceiver *altdest,
+ bool run_once, DestReceiver *dest, DestReceiver *altdest,
char *completionTag);
extern uint64 PortalRunFetch(Portal portal,
diff --git a/src/include/utils/portal.h b/src/include/utils/portal.h
index dc76acd0a4..e7c5a8bd09 100644
--- a/src/include/utils/portal.h
+++ b/src/include/utils/portal.h
@@ -141,6 +141,7 @@ typedef struct PortalData
/* Features/options */
PortalStrategy strategy; /* see above */
int cursorOptions; /* DECLARE CURSOR option bits */
+ bool run_once; /* portal will only be run once */
/* Status data */
PortalStatus status; /* see above */
On Wed, Mar 22, 2017 at 3:00 AM, Rafia Sabih <rafia.sabih@enterprisedb.com>
wrote:
On Wed, Mar 22, 2017 at 12:55 AM, Robert Haas <robertmhaas@gmail.com>
wrote:On Tue, Mar 21, 2017 at 6:36 AM, Dilip Kumar <dilipbalaut@gmail.com>
wrote:
How about taking the decision of execute_once based on
fcache->returnsSet instead of based on lazyEval?change + ExecutorRun(es->qd, ForwardScanDirection, count, !es->lazyEval); to + ExecutorRun(es->qd, ForwardScanDirection, count, !fcache->returnsSet);IMHO, Robert have the same thing in mind?
Yeah, something like that.
Done in execute-once-v2.patch
So, let's see here. We are safe so long as we're sure that, when
postquel_getnext() returns, postquel_end() will be called next without
iterating the loop in fmgr_sql(). That will definitely be true if
fcache->returnsSet is true. It will also be true if postquel_getnext
returns true, which will be true whenever count == 0, which will be true
whenever es->lazyEval is false.
So couldn't we actually make this test !fcache->returnsSet ||
!es->lazyEval? That would let us allow parallel execution for all
non-set-returning functions, and also for set-returning functions that end
up with es->lazyEval set to false.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, Mar 22, 2017 at 10:33 PM, Robert Haas <robertmhaas@gmail.com> wrote:
So couldn't we actually make this test !fcache->returnsSet || !es->lazyEval?
That would let us allow parallel execution for all non-set-returning
functions, and also for set-returning functions that end up with
es->lazyEval set to false.
Yes, this is the right thing to do although we may not enable
parallelism for any more queries by adding "|| !es->lazyEval". Because
SELECT are always marked as es->lazyEval=true(And so far we have
parallelism only for select). But here we calling the parameter to
ExecutorRun as execute_once so !fcache->returnsSet || !es->lazyEval
is the correct one and future proof.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Mar 23, 2017 at 5:23 AM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Wed, Mar 22, 2017 at 10:33 PM, Robert Haas <robertmhaas@gmail.com> wrote:
So couldn't we actually make this test !fcache->returnsSet || !es->lazyEval?
That would let us allow parallel execution for all non-set-returning
functions, and also for set-returning functions that end up with
es->lazyEval set to false.Yes, this is the right thing to do although we may not enable
parallelism for any more queries by adding "|| !es->lazyEval". Because
SELECT are always marked as es->lazyEval=true(And so far we have
parallelism only for select). But here we calling the parameter to
ExecutorRun as execute_once so !fcache->returnsSet || !es->lazyEval
is the correct one and future proof.
Agree, done.
--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/
Attachments:
execute-once-v3.patchapplication/octet-stream; name=execute-once-v3.patchDownload
diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c
index 34b9f1543e..9213ffb6a4 100644
--- a/contrib/auto_explain/auto_explain.c
+++ b/contrib/auto_explain/auto_explain.c
@@ -61,7 +61,7 @@ void _PG_fini(void);
static void explain_ExecutorStart(QueryDesc *queryDesc, int eflags);
static void explain_ExecutorRun(QueryDesc *queryDesc,
ScanDirection direction,
- uint64 count);
+ uint64 count, bool execute_once);
static void explain_ExecutorFinish(QueryDesc *queryDesc);
static void explain_ExecutorEnd(QueryDesc *queryDesc);
@@ -257,15 +257,16 @@ explain_ExecutorStart(QueryDesc *queryDesc, int eflags)
* ExecutorRun hook: all we need do is track nesting depth
*/
static void
-explain_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, uint64 count)
+explain_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction,
+ uint64 count, bool execute_once)
{
nesting_level++;
PG_TRY();
{
if (prev_ExecutorRun)
- prev_ExecutorRun(queryDesc, direction, count);
+ prev_ExecutorRun(queryDesc, direction, count, execute_once);
else
- standard_ExecutorRun(queryDesc, direction, count);
+ standard_ExecutorRun(queryDesc, direction, count, execute_once);
nesting_level--;
}
PG_CATCH();
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 221ac98d4a..42f43233f8 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -290,7 +290,7 @@ static void pgss_post_parse_analyze(ParseState *pstate, Query *query);
static void pgss_ExecutorStart(QueryDesc *queryDesc, int eflags);
static void pgss_ExecutorRun(QueryDesc *queryDesc,
ScanDirection direction,
- uint64 count);
+ uint64 count, bool execute_once);
static void pgss_ExecutorFinish(QueryDesc *queryDesc);
static void pgss_ExecutorEnd(QueryDesc *queryDesc);
static void pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
@@ -871,15 +871,16 @@ pgss_ExecutorStart(QueryDesc *queryDesc, int eflags)
* ExecutorRun hook: all we need do is track nesting depth
*/
static void
-pgss_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, uint64 count)
+pgss_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, uint64 count,
+ bool execute_once)
{
nested_level++;
PG_TRY();
{
if (prev_ExecutorRun)
- prev_ExecutorRun(queryDesc, direction, count);
+ prev_ExecutorRun(queryDesc, direction, count, execute_once);
else
- standard_ExecutorRun(queryDesc, direction, count);
+ standard_ExecutorRun(queryDesc, direction, count, execute_once);
nested_level--;
}
PG_CATCH();
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index ba89b292d1..54aba9156a 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2067,7 +2067,7 @@ CopyTo(CopyState cstate)
else
{
/* run the plan --- the dest receiver will send tuples */
- ExecutorRun(cstate->queryDesc, ForwardScanDirection, 0L);
+ ExecutorRun(cstate->queryDesc, ForwardScanDirection, 0L, true);
processed = ((DR_copy *) cstate->queryDesc->dest)->processed;
}
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 646a88409f..3daffc894a 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -347,7 +347,7 @@ ExecCreateTableAs(CreateTableAsStmt *stmt, const char *queryString,
ExecutorStart(queryDesc, GetIntoRelEFlags(into));
/* run the plan to completion */
- ExecutorRun(queryDesc, ForwardScanDirection, 0L);
+ ExecutorRun(queryDesc, ForwardScanDirection, 0L, true);
/* save the rowcount if we're given a completionTag to fill */
if (completionTag)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index c9b55ead3d..b4c7466666 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -530,7 +530,7 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
dir = ForwardScanDirection;
/* run the plan */
- ExecutorRun(queryDesc, dir, 0L);
+ ExecutorRun(queryDesc, dir, 0L, true);
/* run cleanup too */
ExecutorFinish(queryDesc);
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 86a84ee234..5a84bedf46 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -742,7 +742,7 @@ execute_sql_string(const char *sql, const char *filename)
dest, NULL, 0);
ExecutorStart(qdesc, 0);
- ExecutorRun(qdesc, ForwardScanDirection, 0);
+ ExecutorRun(qdesc, ForwardScanDirection, 0, true);
ExecutorFinish(qdesc);
ExecutorEnd(qdesc);
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index a18c917336..ad39df2024 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -404,7 +404,7 @@ refresh_matview_datafill(DestReceiver *dest, Query *query,
ExecutorStart(queryDesc, EXEC_FLAG_WITHOUT_OIDS);
/* run the plan */
- ExecutorRun(queryDesc, ForwardScanDirection, 0L);
+ ExecutorRun(queryDesc, ForwardScanDirection, 0L, true);
/* and clean up */
ExecutorFinish(queryDesc);
diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c
index 29d0430dd8..f57cf87e8c 100644
--- a/src/backend/commands/portalcmds.c
+++ b/src/backend/commands/portalcmds.c
@@ -395,7 +395,7 @@ PersistHoldablePortal(Portal portal)
true);
/* Fetch the result set into the tuplestore */
- ExecutorRun(queryDesc, ForwardScanDirection, 0L);
+ ExecutorRun(queryDesc, ForwardScanDirection, 0L, false);
(*queryDesc->dest->rDestroy) (queryDesc->dest);
queryDesc->dest = NULL;
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 1cf0d2b971..992ba1c9a2 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -301,7 +301,7 @@ ExecuteQuery(ExecuteStmt *stmt, IntoClause *intoClause,
*/
PortalStart(portal, paramLI, eflags, GetActiveSnapshot());
- (void) PortalRun(portal, count, false, dest, dest, completionTag);
+ (void) PortalRun(portal, count, false, true, dest, dest, completionTag);
PortalDrop(portal, false);
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index f5cd65d8a0..4f5b7b3bca 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -85,7 +85,8 @@ static void ExecutePlan(EState *estate, PlanState *planstate,
bool sendTuples,
uint64 numberTuples,
ScanDirection direction,
- DestReceiver *dest);
+ DestReceiver *dest,
+ bool execute_once);
static bool ExecCheckRTEPerms(RangeTblEntry *rte);
static bool ExecCheckRTEPermsModified(Oid relOid, Oid userid,
Bitmapset *modifiedCols,
@@ -288,17 +289,18 @@ standard_ExecutorStart(QueryDesc *queryDesc, int eflags)
*/
void
ExecutorRun(QueryDesc *queryDesc,
- ScanDirection direction, uint64 count)
+ ScanDirection direction, uint64 count,
+ bool execute_once)
{
if (ExecutorRun_hook)
- (*ExecutorRun_hook) (queryDesc, direction, count);
+ (*ExecutorRun_hook) (queryDesc, direction, count, execute_once);
else
- standard_ExecutorRun(queryDesc, direction, count);
+ standard_ExecutorRun(queryDesc, direction, count, execute_once);
}
void
standard_ExecutorRun(QueryDesc *queryDesc,
- ScanDirection direction, uint64 count)
+ ScanDirection direction, uint64 count, bool execute_once)
{
EState *estate;
CmdType operation;
@@ -345,6 +347,11 @@ standard_ExecutorRun(QueryDesc *queryDesc,
* run plan
*/
if (!ScanDirectionIsNoMovement(direction))
+ {
+ if (execute_once && queryDesc->already_executed)
+ elog(ERROR, "can't re-execute query flagged for single execution");
+ queryDesc->already_executed = true;
+
ExecutePlan(estate,
queryDesc->planstate,
queryDesc->plannedstmt->parallelModeNeeded,
@@ -352,7 +359,9 @@ standard_ExecutorRun(QueryDesc *queryDesc,
sendTuples,
count,
direction,
- dest);
+ dest,
+ execute_once);
+ }
/*
* shutdown tuple receiver, if we started it
@@ -1575,7 +1584,8 @@ ExecutePlan(EState *estate,
bool sendTuples,
uint64 numberTuples,
ScanDirection direction,
- DestReceiver *dest)
+ DestReceiver *dest,
+ bool execute_once)
{
TupleTableSlot *slot;
uint64 current_tuple_count;
@@ -1591,12 +1601,12 @@ ExecutePlan(EState *estate,
estate->es_direction = direction;
/*
- * If a tuple count was supplied, we must force the plan to run without
- * parallelism, because we might exit early. Also disable parallelism
- * when writing into a relation, because no database changes are allowed
- * in parallel mode.
+ * If the plan might potentially be executed multiple times, we must force
+ * it to run without parallelism, because we might exit early. Also
+ * disable parallelism when writing into a relation, because no database
+ * changes are allowed in parallel mode.
*/
- if (numberTuples || dest->mydest == DestIntoRel)
+ if (!execute_once || dest->mydest == DestIntoRel)
use_parallel_mode = false;
if (use_parallel_mode)
@@ -1667,7 +1677,11 @@ ExecutePlan(EState *estate,
*/
current_tuple_count++;
if (numberTuples && numberTuples == current_tuple_count)
+ {
+ /* Allow nodes to release or shut down resources. */
+ (void) ExecShutdownNode(planstate);
break;
+ }
}
if (use_parallel_mode)
diff --git a/src/backend/executor/execParallel.c b/src/backend/executor/execParallel.c
index a1289e5f12..948a9194d5 100644
--- a/src/backend/executor/execParallel.c
+++ b/src/backend/executor/execParallel.c
@@ -852,7 +852,7 @@ ParallelQueryMain(dsm_segment *seg, shm_toc *toc)
ExecParallelInitializeWorker(queryDesc->planstate, toc);
/* Run the plan */
- ExecutorRun(queryDesc, ForwardScanDirection, 0L);
+ ExecutorRun(queryDesc, ForwardScanDirection, 0L, true);
/* Shut down the executor */
ExecutorFinish(queryDesc);
diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index 2d49a65650..12214f8a15 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -855,7 +855,7 @@ postquel_getnext(execution_state *es, SQLFunctionCachePtr fcache)
/* Run regular commands to completion unless lazyEval */
uint64 count = (es->lazyEval) ? 1 : 0;
- ExecutorRun(es->qd, ForwardScanDirection, count);
+ ExecutorRun(es->qd, ForwardScanDirection, count, !fcache->returnsSet || !es->lazyEval);
/*
* If we requested run to completion OR there was no tuple returned,
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 55f97b14e6..72c7b4d068 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2305,7 +2305,7 @@ _SPI_pquery(QueryDesc *queryDesc, bool fire_triggers, uint64 tcount)
ExecutorStart(queryDesc, eflags);
- ExecutorRun(queryDesc, ForwardScanDirection, tcount);
+ ExecutorRun(queryDesc, ForwardScanDirection, tcount, true);
_SPI_current->processed = queryDesc->estate->es_processed;
_SPI_current->lastoid = queryDesc->estate->es_lastoid;
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index b07d6c6cb9..707e7c385d 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -1101,6 +1101,7 @@ exec_simple_query(const char *query_string)
(void) PortalRun(portal,
FETCH_ALL,
isTopLevel,
+ true,
receiver,
receiver,
completionTag);
@@ -1985,6 +1986,7 @@ exec_execute_message(const char *portal_name, long max_rows)
completed = PortalRun(portal,
max_rows,
true, /* always top level */
+ !execute_is_fetch && max_rows == FETCH_ALL,
receiver,
receiver,
completionTag);
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index 371d7350b7..f538b7787c 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -90,6 +90,9 @@ CreateQueryDesc(PlannedStmt *plannedstmt,
qd->planstate = NULL;
qd->totaltime = NULL;
+ /* not yet executed */
+ qd->already_executed = false;
+
return qd;
}
@@ -152,7 +155,7 @@ ProcessQuery(PlannedStmt *plan,
/*
* Run the plan to completion.
*/
- ExecutorRun(queryDesc, ForwardScanDirection, 0L);
+ ExecutorRun(queryDesc, ForwardScanDirection, 0L, true);
/*
* Build command completion status string, if caller wants one.
@@ -679,7 +682,7 @@ PortalSetResultFormat(Portal portal, int nFormats, int16 *formats)
* suspended due to exhaustion of the count parameter.
*/
bool
-PortalRun(Portal portal, long count, bool isTopLevel,
+PortalRun(Portal portal, long count, bool isTopLevel, bool run_once,
DestReceiver *dest, DestReceiver *altdest,
char *completionTag)
{
@@ -712,6 +715,10 @@ PortalRun(Portal portal, long count, bool isTopLevel,
*/
MarkPortalActive(portal);
+ /* Set run_once flag. Shouldn't be clear if previously set. */
+ Assert(!portal->run_once || run_once);
+ portal->run_once = run_once;
+
/*
* Set up global portal context pointers.
*
@@ -918,7 +925,8 @@ PortalRunSelect(Portal portal,
else
{
PushActiveSnapshot(queryDesc->snapshot);
- ExecutorRun(queryDesc, direction, (uint64) count);
+ ExecutorRun(queryDesc, direction, (uint64) count,
+ portal->run_once);
nprocessed = queryDesc->estate->es_processed;
PopActiveSnapshot();
}
@@ -957,7 +965,8 @@ PortalRunSelect(Portal portal,
else
{
PushActiveSnapshot(queryDesc->snapshot);
- ExecutorRun(queryDesc, direction, (uint64) count);
+ ExecutorRun(queryDesc, direction, (uint64) count,
+ portal->run_once);
nprocessed = queryDesc->estate->es_processed;
PopActiveSnapshot();
}
@@ -1394,6 +1403,9 @@ PortalRunFetch(Portal portal,
*/
MarkPortalActive(portal);
+ /* If supporting FETCH, portal can't be run-once. */
+ Assert(!portal->run_once);
+
/*
* Set up global portal context pointers.
*/
diff --git a/src/include/executor/execdesc.h b/src/include/executor/execdesc.h
index c99ea81815..87e7ca8508 100644
--- a/src/include/executor/execdesc.h
+++ b/src/include/executor/execdesc.h
@@ -47,6 +47,9 @@ typedef struct QueryDesc
EState *estate; /* executor's query-wide state */
PlanState *planstate; /* tree of per-plan-node state */
+ /* This field is set by ExecutorRun */
+ bool already_executed; /* true if previously executed */
+
/* This is always set NULL by the core system, but plugins can change it */
struct Instrumentation *totaltime; /* total time spent in ExecutorRun */
} QueryDesc;
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 02dbe7b228..7333569552 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -81,7 +81,8 @@ extern PGDLLIMPORT ExecutorStart_hook_type ExecutorStart_hook;
/* Hook for plugins to get control in ExecutorRun() */
typedef void (*ExecutorRun_hook_type) (QueryDesc *queryDesc,
ScanDirection direction,
- uint64 count);
+ uint64 count,
+ bool execute_once);
extern PGDLLIMPORT ExecutorRun_hook_type ExecutorRun_hook;
/* Hook for plugins to get control in ExecutorFinish() */
@@ -176,9 +177,9 @@ extern TupleTableSlot *ExecFilterJunk(JunkFilter *junkfilter,
extern void ExecutorStart(QueryDesc *queryDesc, int eflags);
extern void standard_ExecutorStart(QueryDesc *queryDesc, int eflags);
extern void ExecutorRun(QueryDesc *queryDesc,
- ScanDirection direction, uint64 count);
+ ScanDirection direction, uint64 count, bool execute_once);
extern void standard_ExecutorRun(QueryDesc *queryDesc,
- ScanDirection direction, uint64 count);
+ ScanDirection direction, uint64 count, bool execute_once);
extern void ExecutorFinish(QueryDesc *queryDesc);
extern void standard_ExecutorFinish(QueryDesc *queryDesc);
extern void ExecutorEnd(QueryDesc *queryDesc);
diff --git a/src/include/tcop/pquery.h b/src/include/tcop/pquery.h
index 61c0b3447e..12ff4588c6 100644
--- a/src/include/tcop/pquery.h
+++ b/src/include/tcop/pquery.h
@@ -34,7 +34,7 @@ extern void PortalSetResultFormat(Portal portal, int nFormats,
int16 *formats);
extern bool PortalRun(Portal portal, long count, bool isTopLevel,
- DestReceiver *dest, DestReceiver *altdest,
+ bool run_once, DestReceiver *dest, DestReceiver *altdest,
char *completionTag);
extern uint64 PortalRunFetch(Portal portal,
diff --git a/src/include/utils/portal.h b/src/include/utils/portal.h
index dc76acd0a4..e7c5a8bd09 100644
--- a/src/include/utils/portal.h
+++ b/src/include/utils/portal.h
@@ -141,6 +141,7 @@ typedef struct PortalData
/* Features/options */
PortalStrategy strategy; /* see above */
int cursorOptions; /* DECLARE CURSOR option bits */
+ bool run_once; /* portal will only be run once */
/* Status data */
PortalStatus status; /* see above */
pl_parallel_opt_support_v3.patchapplication/octet-stream; name=pl_parallel_opt_support_v3.patchDownload
diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index 2d49a65650..5ce6a3ddca 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -503,7 +503,7 @@ init_execution_state(List *queryTree_list,
}
else
stmt = pg_plan_query(queryTree,
- fcache->readonly_func ? CURSOR_OPT_PARALLEL_OK : 0,
+ CURSOR_OPT_PARALLEL_OK,
NULL);
/*
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 55f97b14e6..1f6e669327 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -314,7 +314,7 @@ SPI_execute(const char *src, bool read_only, long tcount)
memset(&plan, 0, sizeof(_SPI_plan));
plan.magic = _SPI_PLAN_MAGIC;
- plan.cursor_options = 0;
+ plan.cursor_options = CURSOR_OPT_PARALLEL_OK;
_SPI_prepare_oneshot_plan(src, &plan);
@@ -458,7 +458,7 @@ SPI_execute_with_args(const char *src,
memset(&plan, 0, sizeof(_SPI_plan));
plan.magic = _SPI_PLAN_MAGIC;
- plan.cursor_options = 0;
+ plan.cursor_options = CURSOR_OPT_PARALLEL_OK;
plan.nargs = nargs;
plan.argtypes = argtypes;
plan.parserSetup = NULL;
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 49a4e622ff..86b8c41415 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3031,7 +3031,7 @@ exec_stmt_return_query(PLpgSQL_execstate *estate,
Assert(stmt->dynquery != NULL);
portal = exec_dynquery_with_params(estate, stmt->dynquery,
stmt->params, NULL,
- CURSOR_OPT_PARALLEL_OK);
+ 0);
}
/* Use eval_mcontext for tuple conversion work */
@@ -3627,7 +3627,12 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
{
ListCell *l;
- exec_prepare_plan(estate, expr, 0);
+ /* Allow parallelism if the function is not trigger type. */
+ if (estate->func->fn_is_trigger == PLPGSQL_NOT_TRIGGER)
+ exec_prepare_plan(estate, expr, CURSOR_OPT_PARALLEL_OK);
+ else
+ exec_prepare_plan(estate, expr, 0);
+
stmt->mod_stmt = false;
foreach(l, SPI_plan_get_plan_sources(expr->plan))
{
@@ -5174,7 +5179,7 @@ exec_eval_expr(PLpgSQL_execstate *estate,
* If first time through, create a plan for this expression.
*/
if (expr->plan == NULL)
- exec_prepare_plan(estate, expr, 0);
+ exec_prepare_plan(estate, expr, CURSOR_OPT_PARALLEL_OK);
/*
* If this is a simple expression, bypass SPI and use the executor
@@ -5185,9 +5190,14 @@ exec_eval_expr(PLpgSQL_execstate *estate,
return result;
/*
- * Else do it the hard way via exec_run_select
+ * Else do it the hard way via exec_run_select. Enable parallelism if
+ * function is not trigger type.
*/
- rc = exec_run_select(estate, expr, 2, NULL, false);
+ if (estate->func->fn_is_trigger == PLPGSQL_NOT_TRIGGER)
+ rc = exec_run_select(estate, expr, 2, NULL, true);
+ else
+ rc = exec_run_select(estate, expr, 2, NULL, false);
+
if (rc != SPI_OK_SELECT)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
On Thu, Mar 23, 2017 at 12:41 AM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:
Agree, done.
OK, committed execute-once-v3.patch after some further study. See
also /messages/by-id/CA+TgmoZ_ZuH+auEeeWnmtorPsgc_SmP+XWbDsJ+cWvWBSjNwDQ@mail.gmail.com
which resulted from that study.
Regarding pl_parallel_opt_support_v3.patch, the change to
init_execution_state looks good. Whether or not it's safe to use
parallel query doesn't depend on whether the function is marked
volatile, so the current code is just silly (and, arguably,
readonly_func is misnamed). The changes to spi.c also seem fine; both
of those functions execute the plan to completion and don't allow
cursor operations, so we're good.
The changes to the plpgsql code don't look so good to me. The change
to exec_stmt_return_query fixes the same bug that I mentioned in the
email linked above, but only half of it -- it corrects the RETURN
QUERY EXECUTE case but not the RETURN QUERY case. And it's clearly a
separate change; that part is a bug fix, not an enhancement. Some of
the other changes depend on whether we're in a trigger, which seems
irrelevant to whether we can use parallelism. Even if the outer query
is doing writes, we can still use parallelism for queries inside the
trigger function if warranted. It's probably a rare case to have
queries inside a trigger that are expensive enough to justify such
handling but I don't see the value of putting in special-case logic to
prevent it.
I suspect that code fails to achieve its goals anyway. At the top of
exec_eval_expr(), you call exec_prepare_plan() and unconditionally
pass CURSOR_OPT_PARALLEL_OK, so when that function returns, expr->plan
might now be a parallel plan. If we reach the call to
exec_run_select() further down in that function, and if we happen to
pass false, it's not going to matter, because exec_run_select() is
going to find the plan already initialized.
--
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 Thu, Mar 23, 2017 at 11:26 PM, Robert Haas <robertmhaas@gmail.com> wrote:
The changes to the plpgsql code don't look so good to me. The change
to exec_stmt_return_query fixes the same bug that I mentioned in the
email linked above, but only half of it -- it corrects the RETURN
QUERY EXECUTE case but not the RETURN QUERY case. And it's clearly a
separate change; that part is a bug fix, not an enhancement.
My bad. Since, you have given this as a separate patch in the link
upthread, I suppose there's nothing expected from me regarding this
right now.
Some of
the other changes depend on whether we're in a trigger, which seems
irrelevant to whether we can use parallelism. Even if the outer query
is doing writes, we can still use parallelism for queries inside the
trigger function if warranted. It's probably a rare case to have
queries inside a trigger that are expensive enough to justify such
handling but I don't see the value of putting in special-case logic to
prevent it.
Fixed. I confused it with not allowing parallel workers when update
command is in progress.
I suspect that code fails to achieve its goals anyway. At the top of
exec_eval_expr(), you call exec_prepare_plan() and unconditionally
pass CURSOR_OPT_PARALLEL_OK, so when that function returns, expr->plan
might now be a parallel plan. If we reach the call to
exec_run_select() further down in that function, and if we happen to
pass false, it's not going to matter, because exec_run_select() is
going to find the plan already initialized.
True, fixed.
The attached patch is to be applied over [1]/messages/by-id/CA+TgmoZ_ZuH+auEeeWnmtorPsgc_SmP+XWbDsJ+cWvWBSjNwDQ@mail.gmail.com -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/.
[1]: /messages/by-id/CA+TgmoZ_ZuH+auEeeWnmtorPsgc_SmP+XWbDsJ+cWvWBSjNwDQ@mail.gmail.com -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/
--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/
Attachments:
pl_parallel_opt_support_v4.patchapplication/octet-stream; name=pl_parallel_opt_support_v4.patchDownload
diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index 12214f8a15..ef589278b6 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -503,7 +503,7 @@ init_execution_state(List *queryTree_list,
}
else
stmt = pg_plan_query(queryTree,
- fcache->readonly_func ? CURSOR_OPT_PARALLEL_OK : 0,
+ CURSOR_OPT_PARALLEL_OK,
NULL);
/*
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 72c7b4d068..eeaa4805e4 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -314,7 +314,7 @@ SPI_execute(const char *src, bool read_only, long tcount)
memset(&plan, 0, sizeof(_SPI_plan));
plan.magic = _SPI_PLAN_MAGIC;
- plan.cursor_options = 0;
+ plan.cursor_options = CURSOR_OPT_PARALLEL_OK;
_SPI_prepare_oneshot_plan(src, &plan);
@@ -458,7 +458,7 @@ SPI_execute_with_args(const char *src,
memset(&plan, 0, sizeof(_SPI_plan));
plan.magic = _SPI_PLAN_MAGIC;
- plan.cursor_options = 0;
+ plan.cursor_options = CURSOR_OPT_PARALLEL_OK;
plan.nargs = nargs;
plan.argtypes = argtypes;
plan.parserSetup = NULL;
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 8e836a8149..b7d28a6f95 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3627,7 +3627,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
{
ListCell *l;
- exec_prepare_plan(estate, expr, 0);
+ exec_prepare_plan(estate, expr, CURSOR_OPT_PARALLEL_OK);
stmt->mod_stmt = false;
foreach(l, SPI_plan_get_plan_sources(expr->plan))
{
@@ -5174,7 +5174,7 @@ exec_eval_expr(PLpgSQL_execstate *estate,
* If first time through, create a plan for this expression.
*/
if (expr->plan == NULL)
- exec_prepare_plan(estate, expr, 0);
+ exec_prepare_plan(estate, expr, CURSOR_OPT_PARALLEL_OK);
/*
* If this is a simple expression, bypass SPI and use the executor
On Fri, Mar 24, 2017 at 5:57 AM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:
I suspect that code fails to achieve its goals anyway. At the top of
exec_eval_expr(), you call exec_prepare_plan() and unconditionally
pass CURSOR_OPT_PARALLEL_OK, so when that function returns, expr->plan
might now be a parallel plan. If we reach the call to
exec_run_select() further down in that function, and if we happen to
pass false, it's not going to matter, because exec_run_select() is
going to find the plan already initialized.True, fixed.
The attached patch is to be applied over [1].
After some scrutiny I didn't find anything particularly wrong with
this, with the exception that exec_eval_expr() was passing false as
the parallelOK argument to exec_run_select(), which is inconsistent
with that function's earlier use of CURSOR_OPT_PARALLEL_OK to plan the
same query. I fixed that by ripping out the parallelOK argument
altogether and just passing CURSOR_OPT_PARALLEL_OK when portalP ==
NULL. The only reason I added parallelOK in the first place was
because of that RETURN QUERY stuff which subsequent study has shown to
be misguided.
Committed that way; please let me know if you see any problems.
--
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