Re: BUG #18305: Unexpected error: "WindowFunc not found in subplan target lists" triggered by subqueries

On Thu, 25 Jan 2024 at 18:14, David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Wed, 24 Jan 2024 at 08:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > We could work around point 1 by refusing to pull up subqueries that
> > contain sublinks in their targetlists, but that would be a pretty big
> > change (and, probably, a pessimization of some queries).  I do not
> > consider run-condition optimization to justify that.  Moreover
> > I'm not sure that sublinks are the only thing that could get
> > mutated to a different state in the runCondition than in the main
> > tree.
> >
> > I think the only real way to prevent problems from point 1 is to stop
> > making a copy of the WindowFunc expr.  We need some other way to refer
> > to the WindowFunc's value in the runCondition tree.  Maybe a generated
> > Param would serve?
>
> I'm wondering if it was wrong to put the runCondition field in
> WindowClause. Maybe it should be in WindowFunc instead...
>
> Really the OpExpr that's created in find_window_run_conditions() with
> the WindowFunc as an arg is there to show the Run Condition in
> EXPLAIN. This is what's later stored in WindowAgg.runConditionOrig.
> What we pass to ExecQual in nodeWindowAgg.c is a version of the OpExpr
> with the WindowFunc replaced with a Var so that ExecQual properly
> fetches the just-calculated WindowFunc return value from the slot.  We
> don't want to leave the WindowFunc in the runCondition as ExecQual
> would go and evaluate it.
>
> If WindowFunc allowed a list of a new struct called WindowRunCondition
> with fields "otherarg", "opno", "collation", "wfunc_left" then we
> could construct the OpExpr later either in createplan.c or setrefs.c.
> The EXPLAIN version of that OpExpr could have the WindowFunc and the
> non-EXPLAIN version would have the Var.

Just to assist the discussion here I've drafted a patch along the
lines of the above. See attached

If you think this idea has merit I can try and turn it into something
committable for master.

> Sounds a bit invasive for back branches, but wondering if we couldn't
> just modify window_ntile_support() to reject any ntile args other than
> Consts. count(*), row_number(), rank(), dense_rank(), percent_rank()
> and percent_rank() all can't suffer from this issue as they don't have
> an argument.  count(expr) would need to have something done to stop
> the same issue from occurring. Maybe int8inc_support() could just set
> req->monotonic = MONOTONICFUNC_NONE if the req->window_func has an
> arg, effectively disabling the optimisation for count(expr).

I'm still unsure about the fix for back branches but I'm open to other ideas.

David