Re: Why does create_gather_merge_plan need make_sort?

On Mon, Nov 23, 2020 at 8:19 AM James Coleman <jtc331(at)gmail(dot)com> wrote:
>
> On Sun, Nov 22, 2020 at 5:07 PM Tomas Vondra
> <tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
> >
> > On 11/22/20 10:31 PM, Tom Lane wrote:
> > > Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com> writes:
> > >> On 11/20/20 11:24 PM, James Coleman wrote:
> > >>> While looking at another issue I noticed that create_gather_merge_plan
> > >>> calls make_sort if the subplan isn't sufficiently sorted. In all of
> > >>> the cases I've seen where a gather merge path (not plan) is created
> > >>> the input path is expected to be properly sorted, so I was wondering
> > >>> if anyone happened to know what case is being handled by the make_sort
> > >>> call. Removing it doesn't seem to break any tests.
> > >
> > >> Yeah, I think you're right this is dead code, essentially. We're only
> > >> ever calling create_gather_merge_path() with pathkeys matching the
> > >> subpath. And it's like that on REL_12_STABLE too, i.e. before the
> > >> incremental sort was introduced.
> > >
> > > It's probably there by analogy to the other callers of
> > > prepare_sort_from_pathkeys, which all do at least a conditional
> > > make_sort(). I'd be inclined to leave it there; it's cheap insurance
> > > against somebody weakening the existing behavior.
> > >
> >
> > But how do we know it's safe to actually do the sort there, e.g. in
> > light of the volatility/parallel-safety issues discussed in other threads?
> >
> > I agree the check may be useful, but maybe we should just do elog(ERROR)
> > instead.
>
> That was my thought. I'm not a big fan of maintaining a "this might be
> useful" path particularly when there isn't any case in the code or
> tests at all that exercises it. In other words, not only is it not
> useful [yet], but also we don't actually have any signal to know that
> it works (or keeps working) -- whether through tests or production
> use.
>
> So I'm +1 on turning it into an ERROR log instead, since it seems to
> me that encountering this case would almost certainly represent a bug
> in path generation.

Here's a patch to do that.

James