Re: Combining Aggregates - Mailing list pgsql-hackers
| From | David Rowley |
|---|---|
| Subject | Re: Combining Aggregates |
| Date | |
| Msg-id | CAKJS1f_zuhVc0g884LNfBJte1BqydG3OYbqfCY4=tAj--1bQ8g@mail.gmail.com Whole thread Raw |
| In response to | Re: Combining Aggregates (Robert Haas <robertmhaas@gmail.com>) |
| List | pgsql-hackers |
On 21 January 2016 at 08:06, Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Jan 20, 2016 at 7:38 AM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
> Agreed. So I've attached a version of the patch which does not have any of
> the serialise/deserialise stuff in it.
I re-reviewed this and have committed most of it with only minor
kibitizing. A few notes:
Great! Thank you for committing.
- I changed the EXPLAIN code, since it failed to display the aggregate
node's mode of operation in non-text format.
Oops, thanks for fixing.
- I removed some of the planner support, since I'm not sure that it's
completely correct and I'm very sure that it contains more code
duplication than I'm comfortable with (set_combineagg_references in
particular). Please feel free to resubmit this part, perhaps in
combination with code that actually uses it for something. But please
also think about whether there's a way to reduce the duplication, and
maybe consider adding some comments, too.
That part is a gray area. Wasn't that sure it belonged to this patch either. The problem is, that without it an Aggref's return type is wrong when the node is in combine mode, which might not be a problem now as we don't have a planner yet that generates these nodes.
- I'm not sure that you made the right call regarding reusing
transfn_oid and transfn for combine functions, vs. adding separate
fields. It is sort of logical and has the advantage of keeping the
data structure smaller, but it might also be thought confusing or
inappropriate on other grounds. But I think we can change it later if
that comes to seem like the right thing to do, so I've left it the way
you had it for now.
Sharing the variable, as horrid as that might seem does simplify some code. For example if you look at find_compatible_pertrans(), then you see:
if (aggtransfn != pertrans->transfn_oid ||
aggtranstype != pertrans->aggtranstype)
continue;
This would need to be changed to something like:
if (!aggstate->combineStates &&
(aggtransfn != pertrans->transfn_oid ||
aggtranstype != pertrans->aggtranstype))
continue;
else if (aggstate->combineStates &&
(aggcombinefn != pertrans->combinefn_oid ||
aggtranstype != pertrans->aggtranstype))
continue;
And we'd then have to pass aggcombinefn to that function too.
What might be better would be to rename the variable to something name that screams something a bit more generic.
--
pgsql-hackers by date: