Better solution to final adjustment of combining Aggrefs

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)postgreSQL(dot)org
Subject: Better solution to final adjustment of combining Aggrefs
Date: 2016-06-25 17:30:03
Message-ID: 15693.1466875803@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I complained earlier about the brute-force way that the partial
aggregation patch deals with constructing Aggrefs for the upper stage of
aggregation. It copied-and-pasted several hundred lines of setrefs.c
so as to inject a nonstandard rule for comparing upper and lower Aggrefs.
That's bulky and will create a constant risk of omissions in future
maintenance. I felt there had to be a better way to do that, and after
some thought I propose the attached not-quite-complete patch.

Basically what this does is to explicitly construct the representation of
an upper combining Aggref with a lower partial Aggref as input. After
that, the regular set_upper_references pass can match the lower partial
Aggref to what it will find in the output tlist of the child plan node,
producing the desired result of a combining Aggref with a Var as input.

The objection that could be raised to this is that it's slightly less
efficient than the original code, since it requires an additional
mutation pass over the tlist and qual of a combining Agg node. I think
that is negligible, though, in comparison to all the other setup costs
of a parallel aggregation plan.

The patch is not quite finished: as noted in the XXX comment, it'd be
a good idea to refactor apply_partialaggref_adjustment so that it can
share code with this function, to ensure they produce identical
representations of the lower partial Aggref. But that will just make
the patch longer, not any more interesting, so I left it out for now.

Another bit of followon work is to get rid of Aggref.aggoutputtype,
which I've also complained about and which is no longer particularly
necessary. I'm inclined to do that in the same commit that adjusts
the partial-aggregation-control fields in Aggref as per the thread at
https://www.postgresql.org/message-id/29309.1466699160@sss.pgh.pa.us

Comments/objections?

regards, tom lane

Attachment Content-Type Size
cleaner-setrefs-handling-of-partial-aggs.patch text/x-diff 15.7 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2016-06-25 22:12:12 Re: Better solution to final adjustment of combining Aggrefs
Previous Message Tom Lane 2016-06-25 16:07:50 Re: Rethinking representation of partial-aggregate steps