Re: segfault with incremental sort

On Tue, Nov 3, 2020 at 1:48 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> James Coleman <jtc331(at)gmail(dot)com> writes:
> > On Tue, Nov 3, 2020 at 11:52 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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