Thread: segfault with incremental sort

luis.roberto@siscobra.com.br writes:
> I'm running a query that is causing the server to segfault.

Hm, can you get a stack trace from that?

https://wiki.postgresql.org/wiki/Generating_a_stack_trace_of_a_PostgreSQL_backend

            regards, tom lane

luis.roberto@siscobra.com.br writes:
> I'm not sure how to get the stack trace correctly.. following the instructions I only get the function names, but not
filesor line numbers...  
> What can I do to get more helpful information?

That means you need to install debug symbols, which on most Linux
systems are an optional sub-package for each package.  On Red Hat
based systems, try "sudo debuginfo-install postgresql"

            regards, tom lane

luis.roberto@siscobra.com.br writes:
> I also managed to find a coredump from systemd, if you want,I can send a link... it's 290MB, though.

Not much use in that, nobody else could do anything without the
matching debug symbols either.

> I can see messages like this, using journalctl -xe:

> Stack trace of thread 548078:
> #0 0x0000000000665ee4 ExecSubPlan (/usr/pgsql-13/bin/postgres)

Hm ... suggestive, but not really enough to debug it.

Can you build a self-contained test case?

            regards, tom lane

luis.roberto@siscobra.com.br writes:
> It took me some time to make it the same... I managed to simplify the error. It appears to be something related to a
subplanwith a distinct clause and another subplan...  

Thanks for the test case!  It looks like this issue is somewhat related
to the "enable_incremental_sort changes query behavior" thread [1].
What I see happening is

1. The SELECT DISTINCT gives rise to a sort key expression that
contains non-parallel-safe SubPlans.  (It's not immediately apparent
to me why we don't consider these particular subqueries parallel safe,
but they aren't.  Anyway such a situation surely has to be allowed for.)

2. The planner ignores the fact that the sort key isn't parallel-safe
and makes a plan with IncrementalSort below Gather anyway.  (I'm not
certain that this bug is specific to IncrementalSort; but given the lack
of previous complaints, I'm guessing we avoid this somehow for regular
sorts.)

3. ExecSerializePlan notes that the subplans aren't parallel-safe and
doesn't send them to the workers.

4. In the workers, nodeSubplan.c dumps core because the planstate it's
expecting is not there.

So, not only do we need to be thinking about volatility while checking
whether IncrementalSort is possible, but also parallel-safety.

In the meantime, now that I've seen this I don't have a lot of confidence
that we'll never inject similar bugs in future.  I'm thinking of
committing the attached to at least reduce the stakes from "core dump"
to "weird error message".

            regards, tom lane

[1]
https://www.postgresql.org/message-id/flat/CAJGNTeNaxpXgBVcRhJX%2B2vSbq%2BF2kJqGBcvompmpvXb7pq%2BoFA%40mail.gmail.com

diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index 9a706df5f0..152c7ae7eb 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -797,7 +797,15 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
     sstate->planstate = (PlanState *) list_nth(estate->es_subplanstates,
                                                subplan->plan_id - 1);

-    /* ... and to its parent's state */
+    /*
+     * This check can fail if the planner mistakenly puts a parallel-unsafe
+     * subplan into a parallelized subquery; see ExecSerializePlan.
+     */
+    if (sstate->planstate == NULL)
+        elog(ERROR, "subplan \"%s\" was not initialized",
+             subplan->plan_name);
+
+    /* Link to parent's state, too */
     sstate->parent = parent;

     /* Initialize subexpressions */

On Tue, Nov 3, 2020 at 11:52 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> luis.roberto@siscobra.com.br writes:
> > It took me some time to make it the same... I managed to simplify the error. It appears to be something related to
asubplan with a distinct clause and another subplan...
 

Thanks for the report, and thanks for reducing it to an easy repro case.

> Thanks for the test case!  It looks like this issue is somewhat related
> to the "enable_incremental_sort changes query behavior" thread [1].
> What I see happening is
>
> 1. The SELECT DISTINCT gives rise to a sort key expression that
> contains non-parallel-safe SubPlans.  (It's not immediately apparent
> to me why we don't consider these particular subqueries parallel safe,
> but they aren't.  Anyway such a situation surely has to be allowed for.)

It's entirely possible that this isn't inherent to incremental sort so
much as the fact that sort (including incremental sort) is now
considered at more places below gather [merge] nodes. I haven't
confirmed that's the case here, but it seems plausible given that that
is the case in the "enable_incremental_sort changes query behavior"
thread you mentioned.

> 2. The planner ignores the fact that the sort key isn't parallel-safe
> and makes a plan with IncrementalSort below Gather anyway.  (I'm not
> certain that this bug is specific to IncrementalSort; but given the lack
> of previous complaints, I'm guessing we avoid this somehow for regular
> sorts.)
>
> 3. ExecSerializePlan notes that the subplans aren't parallel-safe and
> doesn't send them to the workers.
>
> 4. In the workers, nodeSubplan.c dumps core because the planstate it's
> expecting is not there.
>
> So, not only do we need to be thinking about volatility while checking
> whether IncrementalSort is possible, but also parallel-safety.

This is part of why I lean towards guessing it applies to regular sort
also (though haven't confirmed that): the new volatility (and if
volatile, then "expression is in target" checks just mirror existing
code pre-incremental sort.

> In the meantime, now that I've seen this I don't have a lot of confidence
> that we'll never inject similar bugs in future.  I'm thinking of
> committing the attached to at least reduce the stakes from "core dump"
> to "weird error message".

The patch looks good to me.

I also noticed that the incremental sort plan posted earlier has
multiple Gather Merge nodes; that's not what I would have expected,
but maybe I'm missing something. In particular, maybe that's legal
with subplans?

James

James Coleman <jtc331@gmail.com> writes:
> On Tue, Nov 3, 2020 at 11:52 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> So, not only do we need to be thinking about volatility while checking
>> whether IncrementalSort is possible, but also parallel-safety.

> This is part of why I lean towards guessing it applies to regular sort
> also (though haven't confirmed that): the new volatility (and if
> volatile, then "expression is in target" checks just mirror existing
> code pre-incremental sort.

It's certainly possible that the bug exists for non-incremental sort
but previously we'd not try to generate a plan that tripped over it.
Anyway I do not recall seeing code that would specifically check for
this.  I think that, to the extent we've avoided it, it's because the
sort key would normally be part of the reltarget list for some relation
that would thereby get marked non-parallel-safe.  Maybe the DISTINCT
sort key never ends up in any reltarget list?

> I also noticed that the incremental sort plan posted earlier has
> multiple Gather Merge nodes; that's not what I would have expected,
> but maybe I'm missing something.

Hm.  There is only one Gather Merge in the repro case.

            regards, tom lane

On Tue, Nov 3, 2020 at 11:52 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> 1. The SELECT DISTINCT gives rise to a sort key expression that
> contains non-parallel-safe SubPlans.  (It's not immediately apparent
> to me why we don't consider these particular subqueries parallel safe,
> but they aren't.  Anyway such a situation surely has to be allowed for.)

parallel.sgml says that parallel query is excluded any time we have
"Plan nodes which reference a correlated SubPlan". That would include
this query, though I'm not sure why that's actually unsafe. I haven't
thought much about the general case, but this query itself looks like
it'd be safe.

James

On Sat, Nov 21, 2020 at 1:21 AM James Coleman <jtc331@gmail.com> wrote:
>
> On Tue, Nov 3, 2020 at 11:52 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > 1. The SELECT DISTINCT gives rise to a sort key expression that
> > contains non-parallel-safe SubPlans.  (It's not immediately apparent
> > to me why we don't consider these particular subqueries parallel safe,
> > but they aren't.  Anyway such a situation surely has to be allowed for.)
>
> parallel.sgml says that parallel query is excluded any time we have
> "Plan nodes which reference a correlated SubPlan". That would include
> this query, though I'm not sure why that's actually unsafe. I haven't
> thought much about the general case, but this query itself looks like
> it'd be safe.
>

IIRC, the reason was that for correlated subplans each time we need to
send the param for execution to workers, and for that, we don't have
an implementation yet. Basically, if the param size changes each time
(say for varchar type of params), the amount of memory required would
be different each time. It is not that we can't implement it but I
think we have left it originally because we were not sure of the
number of cases it can benefit and certainly it needs some more work.
I am not following this and related discussions closely but can
explain to me why you think the query/plan you are talking about is
safe with respect to the above hazard?

-- 
With Regards,
Amit Kapila.

On Tue, Nov 3, 2020 at 1:48 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> James Coleman <jtc331@gmail.com> writes:
> > On Tue, Nov 3, 2020 at 11:52 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> So, not only do we need to be thinking about volatility while checking
> >> whether IncrementalSort is possible, but also parallel-safety.
>
> > This is part of why I lean towards guessing it applies to regular sort
> > also (though haven't confirmed that): the new volatility (and if
> > volatile, then "expression is in target" checks just mirror existing
> > code pre-incremental sort.
>
> It's certainly possible that the bug exists for non-incremental sort
> but previously we'd not try to generate a plan that tripped over it.
> Anyway I do not recall seeing code that would specifically check for
> this.  I think that, to the extent we've avoided it, it's because the
> sort key would normally be part of the reltarget list for some relation
> that would thereby get marked non-parallel-safe.  Maybe the DISTINCT
> sort key never ends up in any reltarget list?

While first writing this draft I had a fairly long discussion/example
plan about whether or not it was OK that all of the subpaths (nested
loop and below) are marked parallel safe, but I've subsequently
convinced myself that that's correct even though the pathkeys aren't,
because in this case the parallel-unsafe pathkey is a subplan that
hasn't actually yet been set to be evaluated in the subpath.

I'm a bit fuzzy on how subplans (I know it's not a plan yet, but
saying it this way to distinguish it more clearly) are tracked/linked
in a path.

So I decided to include this shortened explanation for future
reference (and to see if there's something that needs correcting in my
understanding).

> > I also noticed that the incremental sort plan posted earlier has
> > multiple Gather Merge nodes; that's not what I would have expected,
> > but maybe I'm missing something.
>
> Hm.  There is only one Gather Merge in the repro case.

I'm able to reproduce having a gather merge underneath each side of a
merge right join with a related bugfix patch applied (0001 in [1]).
But I didn't know if this was a big no-no, or if it's just rare, and
so a bit unexpected, but not necessarily incorrect. What we _don't_
have is a gather merge underneath a gather merge, which is what I
think would definitely be incorrect.

James

1: https://www.postgresql.org/message-id/CAAaqYe8cK3g5CfLC4w7bs%3DhC0mSksZC%3DH5M8LSchj5e5OxpTAg%40mail.gmail.com

On Mon, Nov 23, 2020 at 7:56 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sat, Nov 21, 2020 at 1:21 AM James Coleman <jtc331@gmail.com> wrote:
> >
> > On Tue, Nov 3, 2020 at 11:52 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >
> > > 1. The SELECT DISTINCT gives rise to a sort key expression that
> > > contains non-parallel-safe SubPlans.  (It's not immediately apparent
> > > to me why we don't consider these particular subqueries parallel safe,
> > > but they aren't.  Anyway such a situation surely has to be allowed for.)
> >
> > parallel.sgml says that parallel query is excluded any time we have
> > "Plan nodes which reference a correlated SubPlan". That would include
> > this query, though I'm not sure why that's actually unsafe. I haven't
> > thought much about the general case, but this query itself looks like
> > it'd be safe.
> >
>
> IIRC, the reason was that for correlated subplans each time we need to
> send the param for execution to workers, and for that, we don't have
> an implementation yet. Basically, if the param size changes each time
> (say for varchar type of params), the amount of memory required would
> be different each time. It is not that we can't implement it but I
> think we have left it originally because we were not sure of the
> number of cases it can benefit and certainly it needs some more work.
> I am not following this and related discussions closely but can
> explain to me why you think the query/plan you are talking about is
> safe with respect to the above hazard?

Thanks for the explanation.

In this particular case we're not dealing with variable length fields
(it's an int), so that particular problem wouldn't inherently apply
(though I understand the generalized rule).

But I'm not really quite sure how sending params to workers (from the
leader I assume) is relevant here. In another thread you can see the
full plan [1], but effectively we have:

Gather Merge
  Sort
    Nested Loop
      Bitmap Heap Scan
      Index Only Scan
      Subplan 1
      Subplan 2

where the two subplans are returning the single result of a correlated
subquery (in a SELECT). As I understand it this doesn't require any
coordination with/from the leader at all; all of the information in
this case should actually be local to the individual worker. Each
worker needs to, for each tuple in its index scan, do another index
lookup based on that tuple.

James

1: https://www.postgresql.org/message-id/attachment/116274/explain_verbose.txt

On Mon, Nov 23, 2020 at 7:12 PM James Coleman <jtc331@gmail.com> wrote:
>
> On Tue, Nov 3, 2020 at 1:48 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > James Coleman <jtc331@gmail.com> writes:
> > > I also noticed that the incremental sort plan posted earlier has
> > > multiple Gather Merge nodes; that's not what I would have expected,
> > > but maybe I'm missing something.
> >
> > Hm.  There is only one Gather Merge in the repro case.
>
> I'm able to reproduce having a gather merge underneath each side of a
> merge right join with a related bugfix patch applied (0001 in [1]).
> But I didn't know if this was a big no-no, or if it's just rare, and
> so a bit unexpected, but not necessarily incorrect.
>

I also think such a plan should be fine and shouldn't result in any error.

> What we _don't_
> have is a gather merge underneath a gather merge, which is what I
> think would definitely be incorrect.
>

Right.

-- 
With Regards,
Amit Kapila.

On Mon, Nov 23, 2020 at 7:23 PM James Coleman <jtc331@gmail.com> wrote:
>
> On Mon, Nov 23, 2020 at 7:56 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Sat, Nov 21, 2020 at 1:21 AM James Coleman <jtc331@gmail.com> wrote:
> >
> > IIRC, the reason was that for correlated subplans each time we need to
> > send the param for execution to workers, and for that, we don't have
> > an implementation yet. Basically, if the param size changes each time
> > (say for varchar type of params), the amount of memory required would
> > be different each time. It is not that we can't implement it but I
> > think we have left it originally because we were not sure of the
> > number of cases it can benefit and certainly it needs some more work.
> > I am not following this and related discussions closely but can
> > explain to me why you think the query/plan you are talking about is
> > safe with respect to the above hazard?
>
> Thanks for the explanation.
>
> In this particular case we're not dealing with variable length fields
> (it's an int), so that particular problem wouldn't inherently apply
> (though I understand the generalized rule).
>
> But I'm not really quite sure how sending params to workers (from the
> leader I assume) is relevant here. In another thread you can see the
> full plan [1], but effectively we have:
>
> Gather Merge
>   Sort
>     Nested Loop
>       Bitmap Heap Scan
>       Index Only Scan
>       Subplan 1
>       Subplan 2
>
> where the two subplans are returning the single result of a correlated
> subquery (in a SELECT). As I understand it this doesn't require any
> coordination with/from the leader at all; all of the information in
> this case should actually be local to the individual worker. Each
> worker needs to, for each tuple in its index scan, do another index
> lookup based on that tuple.
>

Yeah, this doesn't require any communication between leader and worker
but to enable it for such cases, we need to identify when we have
correlated subquery where we can make it parallel-safe and then
probably allow it. IIRC, we only allow cases of un-correlated subplans
and initplans when those are at the same or a level above the Gather
(Merge) node. Now, I think it is quite possible that in PG-13 we have
unintentionally allowed plans containing correlated sub-queries but
missed to take care of it at all required places. So, that could be
the reason why we are not seeing any such problems before PG-13? To
prove/disprove this theory, we need to see if we can produce a similar
problem in PG-12 where we don't have incremental_sort or maybe in
PG-13 by disabling incremental_sort. If we have introduced it with
incremental_sort, then we need to either ensure that such plans are
not parallel-safe as would be the case before PG-13 or see what else
needs to be done to support in all such possible cases.

-- 
With Regards,
Amit Kapila.

On Mon, Nov 23, 2020 at 9:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Nov 23, 2020 at 7:23 PM James Coleman <jtc331@gmail.com> wrote:
> >
> > On Mon, Nov 23, 2020 at 7:56 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Sat, Nov 21, 2020 at 1:21 AM James Coleman <jtc331@gmail.com> wrote:
> > >
> > > IIRC, the reason was that for correlated subplans each time we need to
> > > send the param for execution to workers, and for that, we don't have
> > > an implementation yet. Basically, if the param size changes each time
> > > (say for varchar type of params), the amount of memory required would
> > > be different each time. It is not that we can't implement it but I
> > > think we have left it originally because we were not sure of the
> > > number of cases it can benefit and certainly it needs some more work.
> > > I am not following this and related discussions closely but can
> > > explain to me why you think the query/plan you are talking about is
> > > safe with respect to the above hazard?
> >
> > Thanks for the explanation.
> >
> > In this particular case we're not dealing with variable length fields
> > (it's an int), so that particular problem wouldn't inherently apply
> > (though I understand the generalized rule).
> >
> > But I'm not really quite sure how sending params to workers (from the
> > leader I assume) is relevant here. In another thread you can see the
> > full plan [1], but effectively we have:
> >
> > Gather Merge
> >   Sort
> >     Nested Loop
> >       Bitmap Heap Scan
> >       Index Only Scan
> >       Subplan 1
> >       Subplan 2
> >
> > where the two subplans are returning the single result of a correlated
> > subquery (in a SELECT). As I understand it this doesn't require any
> > coordination with/from the leader at all; all of the information in
> > this case should actually be local to the individual worker. Each
> > worker needs to, for each tuple in its index scan, do another index
> > lookup based on that tuple.
> >
>
> Yeah, this doesn't require any communication between leader and worker
> but to enable it for such cases, we need to identify when we have
> correlated subquery where we can make it parallel-safe and then
> probably allow it. IIRC, we only allow cases of un-correlated subplans
> and initplans when those are at the same or a level above the Gather
> (Merge) node.

In principle do you see any reason why given:

select distinct
  unique1,
  (select t.unique1 from tenk1 where tenk1.unique1 = t.unique1)
from tenk1 t, generate_series(1, 1000);

it wouldn't be valid to go from the current (with patch from [1]) plan of:

 Unique
   ->  Sort
         Sort Key: t.unique1, ((SubPlan 1))
         ->  Gather
               Workers Planned: 2
               ->  Nested Loop
                     ->  Parallel Index Only Scan using tenk1_unique1 on tenk1 t
                     ->  Function Scan on generate_series
               SubPlan 1
                 ->  Index Only Scan using tenk1_unique1 on tenk1
                       Index Cond: (unique1 = t.unique1)

to this plan?

 Unique
   ->  Gather Merge
         Workers Planned: 2
         ->  Sort
               Sort Key: t.unique1, ((SubPlan 1))
               ->  Nested Loop
                     ->  Parallel Index Only Scan using tenk1_unique1 on tenk1 t
                     ->  Function Scan on generate_series
                     SubPlan 1
                       ->  Index Only Scan using tenk1_unique1 on tenk1
                             Index Cond: (unique1 = t.unique1)

My intuition is that it would be safe (not just in the parallel sense
but also in the sense of safety for pushing down expression evaluation
into the target list to push the sort down), but I want to make sure
that's correct before proceeding.

But I also have a bigger question:

I've been stepping through this in the debugger, and have noticed that
the only reason we aren't selecting the second plan (again, with the
fix from [1]) is that the plan for "Index Only Scan using
tenk1_unique1 on tenk1" when passed into build_subplan has
"plan->parallel_safe = false". Earlier "consider_parallel = false" has
been set on the path by set_rel_consider_parallel because
is_parallel_safe doesn't find any safe_param_ids, and thus the
correlated subquery having a PARAM_EXEC param fails the safe_param_ids
member check in max_parallel_hazard_walker since the param isn't from
this level.

So far that's correct, but then when we come back around to checking
parallel safety of the expression for a parallel sort we call
is_parallel_safe, and the max_parallel_hazard_walker SubPlan case
checks subplan->parallel_safe, finds it false
("max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context)" returns
true), and thus doesn't proceed to actually checking the subplan's
testexpr or args.

That seems a bit of miss to me, because while such a subplan is
parallel unsafe in isolation, it is not in this context. That is, if
we re-run the checks on testexpr and args in this context, then we'll
not find anything parallel unsafe about them (the param can safely be
found in the current context), so we'll be free to execute the subplan
in workers.

> Now, I think it is quite possible that in PG-13 we have
> unintentionally allowed plans containing correlated sub-queries but
> missed to take care of it at all required places.

Yes, we're discussing a fix in [1] for PG13's missing checks for
parallel safety here.

James

1: https://www.postgresql.org/message-id/CAAaqYe8cK3g5CfLC4w7bs%3DhC0mSksZC%3DH5M8LSchj5e5OxpTAg%40mail.gmail.com

On Wed, Nov 25, 2020 at 7:57 AM James Coleman <jtc331@gmail.com> wrote:
>
> On Mon, Nov 23, 2020 at 9:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Nov 23, 2020 at 7:23 PM James Coleman <jtc331@gmail.com> wrote:
> > >
> >
> > Yeah, this doesn't require any communication between leader and worker
> > but to enable it for such cases, we need to identify when we have
> > correlated subquery where we can make it parallel-safe and then
> > probably allow it. IIRC, we only allow cases of un-correlated subplans
> > and initplans when those are at the same or a level above the Gather
> > (Merge) node.
>
> In principle do you see any reason why given:
>
> select distinct
>   unique1,
>   (select t.unique1 from tenk1 where tenk1.unique1 = t.unique1)
> from tenk1 t, generate_series(1, 1000);
>
> it wouldn't be valid to go from the current (with patch from [1]) plan of:
>
>  Unique
>    ->  Sort
>          Sort Key: t.unique1, ((SubPlan 1))
>          ->  Gather
>                Workers Planned: 2
>                ->  Nested Loop
>                      ->  Parallel Index Only Scan using tenk1_unique1 on tenk1 t
>                      ->  Function Scan on generate_series
>                SubPlan 1
>                  ->  Index Only Scan using tenk1_unique1 on tenk1
>                        Index Cond: (unique1 = t.unique1)
>
> to this plan?
>
>  Unique
>    ->  Gather Merge
>          Workers Planned: 2
>          ->  Sort
>                Sort Key: t.unique1, ((SubPlan 1))
>                ->  Nested Loop
>                      ->  Parallel Index Only Scan using tenk1_unique1 on tenk1 t
>                      ->  Function Scan on generate_series
>                      SubPlan 1
>                        ->  Index Only Scan using tenk1_unique1 on tenk1
>                              Index Cond: (unique1 = t.unique1)
>
> My intuition is that it would be safe (not just in the parallel sense
> but also in the sense of safety for pushing down expression evaluation
> into the target list to push the sort down), but I want to make sure
> that's correct before proceeding.
>

I don't see any problem with respect to parallel-safety but why do we
want to generate parallel-plans for correlated sub-queries without
doing more analysis on which kind of cases it would be safe apart from
this particular case?

> But I also have a bigger question:
>
> I've been stepping through this in the debugger, and have noticed that
> the only reason we aren't selecting the second plan (again, with the
> fix from [1]) is that the plan for "Index Only Scan using
> tenk1_unique1 on tenk1" when passed into build_subplan has
> "plan->parallel_safe = false". Earlier "consider_parallel = false" has
> been set on the path by set_rel_consider_parallel because
> is_parallel_safe doesn't find any safe_param_ids, and thus the
> correlated subquery having a PARAM_EXEC param fails the safe_param_ids
> member check in max_parallel_hazard_walker since the param isn't from
> this level.
>
> So far that's correct, but then when we come back around to checking
> parallel safety of the expression for a parallel sort we call
> is_parallel_safe, and the max_parallel_hazard_walker SubPlan case
> checks subplan->parallel_safe, finds it false
> ("max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context)" returns
> true), and thus doesn't proceed to actually checking the subplan's
> testexpr or args.
>
> That seems a bit of miss to me, because while such a subplan is
> parallel unsafe in isolation, it is not in this context.
>

It is possible but we don't that the context unless subplan is marked
parallel-safe. I think if we need to generate parallel-safe plans for
correlated queries, we might want to see how the subplan will be
marked parallel-safe.

> That is, if
> we re-run the checks on testexpr and args in this context, then we'll
> not find anything parallel unsafe about them (the param can safely be
> found in the current context), so we'll be free to execute the subplan
> in workers.
>
> > Now, I think it is quite possible that in PG-13 we have
> > unintentionally allowed plans containing correlated sub-queries but
> > missed to take care of it at all required places.
>
> Yes, we're discussing a fix in [1] for PG13's missing checks for
> parallel safety here.
>

So, are you trying to make such plans (which have correlated
sub-queries) parallel-safe or parallel-unsafe?

-- 
With Regards,
Amit Kapila.

On Wed, Nov 25, 2020 at 9:05 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Nov 25, 2020 at 7:57 AM James Coleman <jtc331@gmail.com> wrote:
> >
> > On Mon, Nov 23, 2020 at 9:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Mon, Nov 23, 2020 at 7:23 PM James Coleman <jtc331@gmail.com> wrote:
> > > >
> > >
> > > Yeah, this doesn't require any communication between leader and worker
> > > but to enable it for such cases, we need to identify when we have
> > > correlated subquery where we can make it parallel-safe and then
> > > probably allow it. IIRC, we only allow cases of un-correlated subplans
> > > and initplans when those are at the same or a level above the Gather
> > > (Merge) node.
> >
> > In principle do you see any reason why given:
> >
> > select distinct
> >   unique1,
> >   (select t.unique1 from tenk1 where tenk1.unique1 = t.unique1)
> > from tenk1 t, generate_series(1, 1000);
> >
> > it wouldn't be valid to go from the current (with patch from [1]) plan of:
> >
> >  Unique
> >    ->  Sort
> >          Sort Key: t.unique1, ((SubPlan 1))
> >          ->  Gather
> >                Workers Planned: 2
> >                ->  Nested Loop
> >                      ->  Parallel Index Only Scan using tenk1_unique1 on tenk1 t
> >                      ->  Function Scan on generate_series
> >                SubPlan 1
> >                  ->  Index Only Scan using tenk1_unique1 on tenk1
> >                        Index Cond: (unique1 = t.unique1)
> >
> > to this plan?
> >
> >  Unique
> >    ->  Gather Merge
> >          Workers Planned: 2
> >          ->  Sort
> >                Sort Key: t.unique1, ((SubPlan 1))
> >                ->  Nested Loop
> >                      ->  Parallel Index Only Scan using tenk1_unique1 on tenk1 t
> >                      ->  Function Scan on generate_series
> >                      SubPlan 1
> >                        ->  Index Only Scan using tenk1_unique1 on tenk1
> >                              Index Cond: (unique1 = t.unique1)
> >
> > My intuition is that it would be safe (not just in the parallel sense
> > but also in the sense of safety for pushing down expression evaluation
> > into the target list to push the sort down), but I want to make sure
> > that's correct before proceeding.
> >
>
> I don't see any problem with respect to parallel-safety but why do we
> want to generate parallel-plans for correlated sub-queries without
> doing more analysis on which kind of cases it would be safe apart from
> this particular case?

I should clarify: the questions in my last email aren't about what we
should do in PG13, but I wanted to make sure I understand the
theoretical/project policy bounds before pursuing additional work
here.

> > But I also have a bigger question:
> >
> > I've been stepping through this in the debugger, and have noticed that
> > the only reason we aren't selecting the second plan (again, with the
> > fix from [1]) is that the plan for "Index Only Scan using
> > tenk1_unique1 on tenk1" when passed into build_subplan has
> > "plan->parallel_safe = false". Earlier "consider_parallel = false" has
> > been set on the path by set_rel_consider_parallel because
> > is_parallel_safe doesn't find any safe_param_ids, and thus the
> > correlated subquery having a PARAM_EXEC param fails the safe_param_ids
> > member check in max_parallel_hazard_walker since the param isn't from
> > this level.
> >
> > So far that's correct, but then when we come back around to checking
> > parallel safety of the expression for a parallel sort we call
> > is_parallel_safe, and the max_parallel_hazard_walker SubPlan case
> > checks subplan->parallel_safe, finds it false
> > ("max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context)" returns
> > true), and thus doesn't proceed to actually checking the subplan's
> > testexpr or args.
> >
> > That seems a bit of miss to me, because while such a subplan is
> > parallel unsafe in isolation, it is not in this context.
> >
>
> It is possible but we don't that the context unless subplan is marked
> parallel-safe. I think if we need to generate parallel-safe plans for
> correlated queries, we might want to see how the subplan will be
> marked parallel-safe.

The one possibility I can imagine right now is maintaining some kind
of flag that marks a subplan as "parallel safe except for params" and
then recheck the params to see if the specific usage is safe. Does
something like that seem reasonable? Other than the param, the subplan
would be marked safe by the existing code.

> > That is, if
> > we re-run the checks on testexpr and args in this context, then we'll
> > not find anything parallel unsafe about them (the param can safely be
> > found in the current context), so we'll be free to execute the subplan
> > in workers.
> >
> > > Now, I think it is quite possible that in PG-13 we have
> > > unintentionally allowed plans containing correlated sub-queries but
> > > missed to take care of it at all required places.
> >
> > Yes, we're discussing a fix in [1] for PG13's missing checks for
> > parallel safety here.
> >
>
> So, are you trying to make such plans (which have correlated
> sub-queries) parallel-safe or parallel-unsafe?

In PG13 we accidentally made this case generate a parallel plan
because we were missing a parallel safety check when choosing a sort
expression. So for PG13's next patch release we'll be looking to get
in that parallel safety check, which will result in this particular
plan being excluded.

Separately I'm looking into why this case wasn't considered parallel
safe anyways (since it obviously is in reality) with the possibility
of working on a patch to improve that situation for PG14.

James

On Wed, Nov 25, 2020 at 8:27 PM James Coleman <jtc331@gmail.com> wrote:
>
> On Wed, Nov 25, 2020 at 9:05 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Nov 25, 2020 at 7:57 AM James Coleman <jtc331@gmail.com> wrote:
> > >
> >
> > It is possible but we don't that the context unless subplan is marked
> > parallel-safe. I think if we need to generate parallel-safe plans for
> > correlated queries, we might want to see how the subplan will be
> > marked parallel-safe.
>
> The one possibility I can imagine right now is maintaining some kind
> of flag that marks a subplan as "parallel safe except for params" and
> then recheck the params to see if the specific usage is safe. Does
> something like that seem reasonable? Other than the param, the subplan
> would be marked safe by the existing code.
>

I don't know. What I remember is it is not easy to check the params to
see if the specific usage is safe for correlated sub-queries as
detecting the level of param was tricky. Basically, whether the param
can be generated below Gather node so that it could be used was a bit
tricky but maybe I had missed something obvious during my analysis.
However, feel free to give it a try.

> > > That is, if
> > > we re-run the checks on testexpr and args in this context, then we'll
> > > not find anything parallel unsafe about them (the param can safely be
> > > found in the current context), so we'll be free to execute the subplan
> > > in workers.
> > >
> > > > Now, I think it is quite possible that in PG-13 we have
> > > > unintentionally allowed plans containing correlated sub-queries but
> > > > missed to take care of it at all required places.
> > >
> > > Yes, we're discussing a fix in [1] for PG13's missing checks for
> > > parallel safety here.
> > >
> >
> > So, are you trying to make such plans (which have correlated
> > sub-queries) parallel-safe or parallel-unsafe?
>
> In PG13 we accidentally made this case generate a parallel plan
> because we were missing a parallel safety check when choosing a sort
> expression. So for PG13's next patch release we'll be looking to get
> in that parallel safety check, which will result in this particular
> plan being excluded.
>

Okay, this was what I was wondering about.

-- 
With Regards,
Amit Kapila.

On Mon, Nov 30, 2020 at 8:56 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Nov 25, 2020 at 8:27 PM James Coleman <jtc331@gmail.com> wrote:
> >
> > On Wed, Nov 25, 2020 at 9:05 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, Nov 25, 2020 at 7:57 AM James Coleman <jtc331@gmail.com> wrote:
> > > >
> > >
> > > It is possible but we don't that the context unless subplan is marked
> > > parallel-safe. I think if we need to generate parallel-safe plans for
> > > correlated queries, we might want to see how the subplan will be
> > > marked parallel-safe.
> >
> > The one possibility I can imagine right now is maintaining some kind
> > of flag that marks a subplan as "parallel safe except for params" and
> > then recheck the params to see if the specific usage is safe. Does
> > something like that seem reasonable? Other than the param, the subplan
> > would be marked safe by the existing code.
> >
>
> I don't know. What I remember is it is not easy to check the params to
> see if the specific usage is safe for correlated sub-queries as
> detecting the level of param was tricky. Basically, whether the param
> can be generated below Gather node so that it could be used was a bit
> tricky but maybe I had missed something obvious during my analysis.
> However, feel free to give it a try.

Thanks. I have some work in progress that seems to show promise so
far; I hope to post about it in a new thread later this week.

James