Re: enable_incremental_sort changes query behavior - Mailing list pgsql-hackers

From James Coleman
Subject Re: enable_incremental_sort changes query behavior
Date
Msg-id CAAaqYe_PySjVytws75-Zm1dv1pK2r6_um7g+S34KHZZR8nUNDg@mail.gmail.com
Whole thread Raw
In response to Re: enable_incremental_sort changes query behavior  (Tomas Vondra <tomas.vondra@enterprisedb.com>)
List pgsql-hackers
On Tue, Nov 17, 2020 at 11:20 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
>
>
> On 11/17/20 3:03 PM, James Coleman wrote:
> > On Mon, Nov 16, 2020 at 11:23 PM Tomas Vondra
> > <tomas.vondra@enterprisedb.com> wrote:
> >>
> >> Hmm, I missed that other thread. That indeed seems like a bug in the
> >> same area already tweaked by ebb7ae839d033d0f2 for similar cases.
> >
> > I meant to bring it up in this thread before we got the other patch
> > committed, but I just ended up not having time to look into it.
> >
> >> The attached patch fixes this simply by adding is_parallel_safe to
> >> get_useful_pathkeys_for_relation - that does fix the reproducer, but I'm
> >> not entirely sure that's the right place. Maybe it should be done in
> >> find_em_expr_usable_for_sorting_rel (which might make a difference if an
> >> EC clause can contain a mix of parallel safe and unsafe expressions). Or
> >> maybe we should do it in the caller (which would allow using
> >> get_useful_pathkeys_for_relation in contexts not requiring parallel
> >> safety in the future).
> >>
> >> Anyway, while this is not an "incremental sort" bug, it seems like the
> >> root cause is the same as for ebb7ae839d033d0f2 - one of the incremental
> >> sort patches started considering sorting below gather nodes, not
> >> realizing these possible consequences.
> >
> > Yeah. I'd like to investigate a bit if really we should also be adding
> > it to prepare_sort_from_pathkeys (which
> > find_em_expr_usable_for_sorting_rel parallels) or similar, since this
> > seems to be a broader concern that's been previously missed (even if
> > the bug was otherwise not yet exposed). In particular, as I understand
> > it, we did do sorts below gather nodes before, just in fewer places,
> > which meant that "in theory" the code was already broken (or at least
> > not complete) for parallel queries.
> >
>
> True. It's possible the bug was there before, but the affected plans
> were just very unlikely for some reason, and the new plans introduced by
> incremental sort just made it easier to trigger.

For additional patches on the broader topic (e.g., parallel safety) is
it preferable to keep them here or in the thread Luis started (or even
a new one besides that -- since that's on -bugs not -hackers)?

James



pgsql-hackers by date:

Previous
From: Peter Eisentraut
Date:
Subject: Re: [PATCH] remove pg_archivecleanup and pg_standby
Next
From: Tom Lane
Date:
Subject: Re: Removal of currtid()/currtid2() and some table AM cleanup