Re: enable_incremental_sort changes query behavior

On Fri, Oct 2, 2020 at 7:07 PM James Coleman <jtc331(at)gmail(dot)com> wrote:
>
> On Fri, Oct 2, 2020 at 6:28 PM Tomas Vondra
> <tomas(dot)vondra(at)2ndquadrant(dot)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(dot)vondra(at)2ndquadrant(dot)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.

I've started poking at this, but haven't yet found a workable
solution. See the attached patch which "solves" the problem by
breaking putting the sort under the gather merge, but it at least
demonstrates conceptually what I think we're interested in doing.

The issue currently is that the comparison of expressions fails -- in
our query, the first column selected shows up as a Var (with the same
contents) but is a different pointer in the em_expr and the reltarget
exprs list. I don't yet see a good way to get the equivalence class
for a PathTarget entry.

James