From: | David Rowley <dgrowleyml(at)gmail(dot)com> |
---|---|
To: | Andres Freund <andres(at)anarazel(dot)de> |
Cc: | Andy Fan <zhihui(dot)fan1213(at)gmail(dot)com>, Zhihong Yu <zyu(at)yugabyte(dot)com>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Window Function "Run Conditions" |
Date: | 2022-04-05 00:04:18 |
Message-ID: | CAApHDvp+rzxGh58hc3Fmr90PR-M=JOg96X0B5tfvY4pTKqNa3g@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Thanks for having a look at this.
On Wed, 30 Mar 2022 at 11:16, Andres Freund <andres(at)anarazel(dot)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
Attachment | Content-Type | Size |
---|---|---|
v5-0001-Teach-planner-and-executor-about-monotonic-window.patch | text/plain | 62.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Jeff Davis | 2022-04-05 00:10:51 | Re: Extensible Rmgr for Table AMs |
Previous Message | Mark Dilger | 2022-04-05 00:01:00 | Re: Granting SET and ALTER SYSTE privileges for GUCs |