Re: Fix generate_useful_gather_paths for parallel unsafe pathkeys

On Sun, Nov 29, 2020 at 4:20 PM Tomas Vondra
<tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>
> 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"?

Done.

> > 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.

IS_SRF_CALL doesn't really seem to be particularly specific
conceptually to target lists (and of course now we're using it in
equivclass.c), so I'd tried to put it somewhere more general. Is
moving it to tlist.h mostly to minimize changes? Or do you think it
fits more naturally with the tlist code (I might be missing
something)?

> > 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?

At this point I'm not sure I'd have a good way to describe that: one
is a clear superset of the other (and we already talk in the comments
about the other being a subset), but it's really going to be about
"what do we want to allow to be sorted proactively"...hmm, that
thought made me take a swing at it; let me know what you think.

James