From: | Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com> |
---|---|
To: | James Coleman <jtc331(at)gmail(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 21:20:39 |
Message-ID: | a6c8907f-6a31-e776-6e48-e8a3ac84d037@enterprisedb.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 11/29/20 3:26 PM, James Coleman wrote:
> 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.
>
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
From | Date | Subject | |
---|---|---|---|
Next Message | Noah Misch | 2020-11-29 21:44:41 | Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions |
Previous Message | Tomas Vondra | 2020-11-29 21:10:03 | Re: Why does create_gather_merge_plan need make_sort? |