Why does create_gather_merge_plan need make_sort?

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Why does create_gather_merge_plan need make_sort?

James Coleman
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.

Thanks,
James


Reply | Threaded
Open this post in threaded view
|

Re: Why does create_gather_merge_plan need make_sort?

Tomas Vondra-6
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.


regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Reply | Threaded
Open this post in threaded view
|

Re: Why does create_gather_merge_plan need make_sort?

Tom Lane-2
Tomas Vondra <[hidden email]> 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.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Why does create_gather_merge_plan need make_sort?

Tomas Vondra-6
On 11/22/20 10:31 PM, Tom Lane wrote:

> Tomas Vondra <[hidden email]> 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.



regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Reply | Threaded
Open this post in threaded view
|

Re: Why does create_gather_merge_plan need make_sort?

James Coleman
On Sun, Nov 22, 2020 at 5:07 PM Tomas Vondra
<[hidden email]> wrote:

>
> On 11/22/20 10:31 PM, Tom Lane wrote:
> > Tomas Vondra <[hidden email]> 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.

James