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
