From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: James Coleman <jtc331@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Cc: luis.roberto@siscobra.com.br, Tom Lane <tgl@sss.pgh.pa.us>
Cc: Robert Haas <robertmhaas@gmail.com>
Subject: Re: Fix generate_useful_gather_paths for parallel unsafe pathkeys
Date: Sun, 29 Nov 2020 22:20:39 +0100
Message-ID: <a6c8907f-6a31-e776-6e48-e8a3ac84d037@enterprisedb.com> (raw)
In-Reply-To: <CAAaqYe8gjjCbE3K068GDg_qSwp530JH44m08xr4-sDTJ3bX9Nw@mail.gmail.com>
References: <CAAaqYe8cK3g5CfLC4w7bs=hC0mSksZC=H5M8LSchj5e5OxpTAg@mail.gmail.com>
<28a4f17e-0b81-8bd5-d40f-e5d69d8101b1@enterprisedb.com>
<CAAaqYe-m04ZGSYgvf3h-i_R6=PBkxSWHAFbAvxH8Y5p0wm1tWw@mail.gmail.com>
<CAAaqYe8gjjCbE3K068GDg_qSwp530JH44m08xr4-sDTJ3bX9Nw@mail.gmail.com>
On 11/29/20 3:26 PM, James Coleman wrote:
> On Mon, Nov 23, 2020 at 8:35 AM James Coleman <jtc331@gmail.com> wrote:
>>
>> On Sun, Nov 22, 2020 at 4:59 PM Tomas Vondra
>> <tomas.vondra@enterprisedb.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.
>
0001 - seems fine
0002 - Should we rename the "parallel" parameter to something more
descriptive, like "require_parallel_safe"?
> 0003 disallows SRFs in these sort expressions (per discussion over at [1]).
>
OK, but I'd move the define from tlist.c to tlist.h, not optimizer.h.
> 0004 removes the search through the target for matching volatile
> expressions (per discussion at [2]).
>
OK
> 0005 adds the comment I mentioned above clarifying these two functions
> are linked.
>
Makes sense. I wonder if we need to be more specific about how
consistent those two places need to be. Do they need to match 1:1, or
how do people in the future decide which restrictions need to be in both
places?
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@enterprisedb.com, jtc331@gmail.com, tgl@sss.pgh.pa.us, robertmhaas@gmail.com
Subject: Re: Fix generate_useful_gather_paths for parallel unsafe pathkeys
In-Reply-To: <a6c8907f-6a31-e776-6e48-e8a3ac84d037@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox