Better solution to final adjustment of combining Aggrefs
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
/messages/by-id/29309.1466699160@sss.pgh.pa.us
Comments/objections?
regards, tom lane
Attachments:
cleaner-setrefs-handling-of-partial-aggs.patchtext/x-diff; charset=us-ascii; name=cleaner-setrefs-handling-of-partial-aggs.patchDownload+155-331
I wrote:
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.
Here's an extended version that includes that refactoring. I ended up
deciding that apply_partialaggref_adjustment was useless: stripped of the
actual Aggref-modification logic, it's little more than an iteration over
a pathtarget list, and the fact that examining the top-level nodes is
sufficient seems like an artifact of its one existing caller rather than
general-purpose functionality. It also had no business being in tlist.c
AFAICS (the fact that putting it there required doubling the length of
tlist.c's #include list should have clued somebody that it didn't belong
there...). So I moved the loop into make_partialgroup_input_target and
created a separate function for the Aggref-munging part.
While at that I noticed that make_partialgroup_input_target was
misleadingly named and documented: what it produces is the output target
for the partial aggregation step, not the input. And for that matter
its argument is not what the rest of planner.c calls the final_target.
So this attempts to clean that up as well.
regards, tom lane