Thread: [HACKERS] 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/

-- 
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:
> 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

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/

-- 
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 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

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

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

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

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

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

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

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

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

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
thecheap 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

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

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

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

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

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

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

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Mar 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

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 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?
>
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],
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] https://www.postgresql.org/message-id/CA+TgmobXEhvHbJtWDuPZM9bVSLiTj-kShxQJ2uM5GPDze9fRYA@mail.gmail.com
-- 
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/

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

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/

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

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

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/

-- 
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 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

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/

-- 
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 12:41 AM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:
> Agree, done.

OK, committed execute-once-v3.patch after some further study.  See
also
https://www.postgresql.org/message-id/CA%2BTgmoZ_ZuH%2BauEeeWnmtorPsgc_SmP%2BXWbDsJ%2BcWvWBSjNwDQ%40mail.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

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].

[1]
https://www.postgresql.org/message-id/CA%2BTgmoZ_ZuH%2BauEeeWnmtorPsgc_SmP%2BXWbDsJ%2BcWvWBSjNwDQ%40mail.gmail.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 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