Re: enable_incremental_sort changes query behavior

On Fri, Oct 2, 2020 at 6:28 PM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
>
> On Fri, Oct 02, 2020 at 05:45:52PM -0400, James Coleman wrote:
> >On Fri, Oct 2, 2020 at 4:56 PM Tomas Vondra
> ><tomas.vondra@2ndquadrant.com> wrote:
> >>
> >> ...
> >>
> >> More importanly, it does not actually fix the issue - it does fix that
> >> particular query, but just replacing the DISTINCT with either ORDER BY
> >> or GROUP BY make it fail again :-(
> >>
> >> Attached is a simple script I used, demonstrating these issues for the
> >> three cases that expect to have ressortgroupref != 0 (per the comment
> >> before TargetEntry in plannodes.h).
> >
> >So does checking for volatile expressions (if you happened to test
> >that) solve all the cases? If you haven't tested that yet, I can try
> >to do that this evening.
> >
>
> Yes, it does fix all the three queries in the SQL script.
>
> The question however is whether this is the root issue, and whether it's
> the right way to fix it. For example - volatility is not the only reason
> that may block qual pushdown. If you look at qual_is_pushdown_safe, it
> also blocks pushdown of leaky functions in security_barrier views. So I
> wonder if that could cause failures too, somehow. But I haven't managed
> to create such example.

I was about to say that the issue here is slightly different from qual
etc. pushdown, since we're not concerned about quals here, and so I
wonder where we determine what target list entries to put in a given
scan path, but then I realized that implies (maybe!) a simpler
solution. Instead of duplicating checks on target list entries would
be safe, why not check directly in get_useful_pathkeys_for_relation()
whether or not the pathkey has a target list entry?

I haven't been able to try that out yet, and so maybe I'm missing
something, but I need to step out for a bit, so I'll have to look at
it later.

James