Subject: Re: Fix generate_useful_gather_paths for parallel unsafe pathkeys
To: James Coleman <jtc331@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>,
 luis.roberto@siscobra.com.br, Tom Lane <tgl@sss.pgh.pa.us>,
 Robert Haas <robertmhaas@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>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
Message-ID: <a6c8907f-6a31-e776-6e48-e8a3ac84d037@enterprisedb.com>
Date: Sun, 29 Nov 2020 22:20:39 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.0
MIME-Version: 1.0
In-Reply-To: 
 <CAAaqYe8gjjCbE3K068GDg_qSwp530JH44m08xr4-sDTJ3bX9Nw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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