Re: Fix generate_useful_gather_paths for parallel unsafe pathkeys

From: James Coleman <jtc331(at)gmail(dot)com>
To: Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, luis(dot)roberto(at)siscobra(dot)com(dot)br, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>
Subject: Re: Fix generate_useful_gather_paths for parallel unsafe pathkeys
Date: 2020-11-29 14:26:12
Message-ID: CAAaqYe8gjjCbE3K068GDg_qSwp530JH44m08xr4-sDTJ3bX9Nw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Mon, Nov 23, 2020 at 8:35 AM James Coleman <jtc331(at)gmail(dot)com> wrote:
>
> On Sun, Nov 22, 2020 at 4:59 PM Tomas Vondra
> <tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
> > ...
> > Thanks for the patches, I'll take a closer look next week. The 0002
> > patch is clearly something we need to backpatch, not sure about 0001 as
> > it essentially enables new behavior in some cases (Sort on unsorted
> > paths below Gather Merge).
>
> Thanks for taking a look.
>
> I had the same question re: backporting. On the one hand it will
> change behavior (this is always a bit of a gray area since in one
> sense bugs change behavior), but IMO it's not a new feature, because
> the code clearly intended to have that feature in the first place (it
> creates gather merges on top of a full sort). So I'd lean towards
> considering it a bug fix, but I'm also not going to make that a hill
> to die on.
>
> One semi-related question: do you think we should add a comment to
> prepare_sort_from_pathkeys explaining that modifying it may mean
> find_em_expr_for_rel needs to be updated also?

Here's an updated patch series.

0001 and 0002 as before, but with some minor cleanup.

0003 disallows SRFs in these sort expressions (per discussion over at [1]).

0004 removes the search through the target for matching volatile
expressions (per discussion at [2]).

0005 adds the comment I mentioned above clarifying these two functions
are linked.

James

1: https://www.postgresql.org/message-id/295524.1606246314%40sss.pgh.pa.us
2: https://www.postgresql.org/message-id/124400.1606159456%40sss.pgh.pa.us

Attachment Content-Type Size
v2-0004-Remove-volatile-expr-target-search.patch text/x-patch 2.4 KB
v2-0003-Disallow-SRFs-in-proactive-sort.patch text/x-patch 4.0 KB
v2-0002-Enforce-parallel-safety-of-pathkeys-in-generate_u.patch text/x-patch 8.1 KB
v2-0005-Document-find_em_expr_usable_for_sorting_rel-in-p.patch text/x-patch 995 bytes
v2-0001-generate_useful_gather_paths-shouldn-t-skip-unsor.patch text/x-patch 3.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message James Coleman 2020-11-29 14:44:33 Re: Why does create_gather_merge_plan need make_sort?
Previous Message Pavel Stehule 2020-11-29 14:09:48 Re: proposal: possibility to read dumped table's name from file