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