Re: segfault with incremental sort

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