Re: Window Function "Run Conditions"

Thanks for having a look at this.

On Wed, 30 Mar 2022 at 11:16, Andres Freund <andres@anarazel.de> wrote:
> On 2022-03-29 15:11:52 +1300, David Rowley wrote:
> > One thing which I'm not sure about with the patch is how I'm handling
> > the evaluation of the runcondition in nodeWindowAgg.c.  Instead of
> > having ExecQual() evaluate an OpExpr such as "row_number() over (...)
> > <= 10", I'm replacing the WindowFunc with the Var in the targetlist
> > that corresponds to the given WindowFunc.  This saves having to double
> > evaluate the WindowFunc. Instead, the value of the Var can be taken
> > directly from the slot.  I don't know of anywhere else we do things
> > quite like that.  The runcondition is slightly similar to HAVING
> > clauses, but HAVING clauses don't work this way.
>
> Don't HAVING clauses actually work pretty similar? Yes, they don't have a Var,
> but for expression evaluation purposes an Aggref is nearly the same as a plain
> Var:
>
>         EEO_CASE(EEOP_INNER_VAR)
>         {
>             int         attnum = op->d.var.attnum;
>
>             /*
>              * Since we already extracted all referenced columns from the
>              * tuple with a FETCHSOME step, we can just grab the value
>              * directly out of the slot's decomposed-data arrays.  But let's
>              * have an Assert to check that that did happen.
>              */
>             Assert(attnum >= 0 && attnum < innerslot->tts_nvalid);
>             *op->resvalue = innerslot->tts_values[attnum];
>             *op->resnull = innerslot->tts_isnull[attnum];
>
>             EEO_NEXT();
>         }
> vs
>         EEO_CASE(EEOP_AGGREF)
>         {
>             /*
>              * Returns a Datum whose value is the precomputed aggregate value
>              * found in the given expression context.
>              */
>             int         aggno = op->d.aggref.aggno;
>
>             Assert(econtext->ecxt_aggvalues != NULL);
>
>             *op->resvalue = econtext->ecxt_aggvalues[aggno];
>             *op->resnull = econtext->ecxt_aggnulls[aggno];
>
>             EEO_NEXT();
>         }
>
> specifically we don't re-evaluate expressions?

Thanks for highlighting the similarities. I'm feeling better about the
choice now.

I've made another pass over the patch and updated a few comments and
made a small code change to delay the initialisation of a variable.

I'm pretty happy with this now. If anyone wants to have a look at
this, can they do so or let me know they're going to within the next
24 hours.  Otherwise I plan to move into commit mode with it.

> This is afaics slightly cheaper than referencing a variable in a slot.

I guess you must mean cheaper because it means there will be no
EEOP_*_FETCHSOME step?  Otherwise it seems a fairly similar amount of
work.

David