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

De: "Tom Lane" <tgl@sss.pgh.pa.us>
Para: "luis.roberto" <luis.roberto@siscobra.com.br>
Cc: "pgsql-bugs" <pgsql-bugs@lists.postgresql.org>, "alan.formagi" <alan.formagi@siscobra.com.br>
Enviadas: Sexta-feira, 30 de outubro de 2020 15:22:02
Assunto: Re: 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
_____________________

Hi

I'm not sure how to get the stack trace correctly.. following the instructions I only get the function names, but not files or line numbers...

What can I do to get more helpful information?

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

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

hmm. CentOS is pointing to a 404, so I can't install it right now..

I managed to strace it, altough I'm not sure it helps. The log is rather large, so I uploaded it: https://mega.nz/file/Y7higI5B#LVnpeGtCkUswOlXXjKW5CnNiLNm4iIdfInBS8fNNzIE

hmm. CentOS is pointing to a 404, so I can't install it right now..
I managed to strace it, altough I'm not sure it helps. The log is rather large, so I uploaded it: https://mega.nz/file/Y7higI5B#LVnpeGtCkUswOlXXjKW5CnNiLNm4iIdfInBS8fNNzIE

I also managed to find a coredump from systemd, if you want,I can send a link... it's 290MB, though.

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

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

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

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

Can you build a self-contained test case?

regards, tom lane

Hi Tom!

It took me some time to make it the same... I managed to simplify the error. It appears to be something related to a subplan with a distinct clause and another subplan...

drop table main,secondary;
create table main (
id bigint generated by default as identity primary key,
id2 int not null,
type smallint default 0,
name text not null
);
insert into main (id2,name,type)
select (id%100)+1,md5(id::text),case when (id%100) > 0 then 0 else 1 end
from generate_series(1,3401305) a(id);

create index on main (id2);

create table secondary (
id bigint,
id2 smallint,
name text,
primary key (id,id2)
);

insert into secondary (id,id2,name)
select m.id,a.seq,md5(m.id::text)
from main m,
generate_series(1,16) a(seq);

analyze main,secondary;

explain analyze
select m.id2,
m.id,
s.description
FROM main m
LEFT JOIN ( SELECT DISTINCT
m.id,
CASE
WHEN m.id2 = 15 AND (SELECT name FROM secondary x WHERE x.id = s2.id AND x.id2 = 10) = md5(123::text) THEN 'description'
WHEN m.id2 = 15 THEN (SELECT name FROM secondary x WHERE x.id = s2.id AND x.id2 = 5)
END AS description
FROM main m
JOIN secondary s2 ON m.id = s2.id
WHERE m.id2 = 15
and type = 0) s ON s.id = m.id
WHERE m.id2 IN (15)
and type = 0

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 subplan with 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]/messages/by-id/CAJGNTeNaxpXgBVcRhJX+2vSbq+F2kJqGBcvompmpvXb7pq+oFA@mail.gmail.com.
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]: /messages/by-id/CAJGNTeNaxpXgBVcRhJX+2vSbq+F2kJqGBcvompmpvXb7pq+oFA@mail.gmail.com

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

Thanks for confirming it. For now, disabling incremental sort will 'solve' the issue. I'll be keeping an eye on that thread.

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 a subplan 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: /messages/by-id/CAAaqYe8cK3g5CfLC4w7bs=hC0mSksZC=H5M8LSchj5e5OxpTAg@mail.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: /messages/by-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: /messages/by-id/CAAaqYe8cK3g5CfLC4w7bs=hC0mSksZC=H5M8LSchj5e5OxpTAg@mail.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.