Re: Combining Aggregates

On 21 January 2016 at 08:06, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Wed, Jan 20, 2016 at 7:38 AM, David Rowley
> <david(dot)rowley(at)2ndquadrant(dot)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.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services