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
