Re: Combining Aggregates

On 22 March 2016 at 05:27, Robert Haas <robertmhaas@gmail.com> wrote:
> On Sat, Mar 19, 2016 at 11:48 PM, David Rowley
> <david.rowley@2ndquadrant.com> wrote:
>> 0002: Adds serial/de-serial function support to CREATE AGGREGATE,
>> contains minor fix-ups from last version.
>
> This looks pretty good, but don't build_aggregate_serialfn_expr and
> build_aggregate_deserialfn_expr compile down to identical machine
> code?  Keeping two copies of the same code with different parameter
> names is a degree of neatnik-ism I'm not sure I can swallow.

Good point. I've removed the deserial one, and renamed the arguments
to arg_input_type and arg_output_type.

> The only caller to make_partialgroup_input_target() passes true for
> the additional argument.  That doesn't seem right.

Yeah, I coded that with non-parallel use cases in mind. I very much
thing we'll end up using that flag in 9.7, but perhaps that's not a
good enough reason to be adding it in 9.6. I've pulled it out of the
patch for now, but I could also go further and pull the flag from the
whole patch, as we could just serialize the states in nodeAgg.c when
finalizeAggs == false and deserialize when combineStates == true.

> Maybe error messages should refer to "aggregate serialization
> function" and "aggregate deserialization function" instead of
> "aggregate serial function" and "aggregate de-serial function".

That's probably a good idea.

> - *       Other behavior is also supported and is controlled by the
> 'combineStates'
> - *       and 'finalizeAggs'. 'combineStates' controls whether the trans func or
> - *       the combine func is used during aggregation.  When 'combineStates' is
> - *       true we expect other (previously) aggregated states as input
> rather than
> - *       input tuples. This mode facilitates multiple aggregate stages which
> - *       allows us to support pushing aggregation down deeper into
> the plan rather
> - *       than leaving it for the final stage. For example with a query such as:
> + *       Other behavior is also supported and is controlled by the
> 'combineStates',
> + *       'finalizeAggs' and 'serialStates' parameters. 'combineStates' controls
> + *       whether the trans func or the combine func is used during aggregation.
> + *       When 'combineStates' is true we expect other (previously) aggregated
> + *       states as input rather than input tuples. This mode
> facilitates multiple
> + *       aggregate stages which allows us to support pushing aggregation down
> + *       deeper into the plan rather than leaving it for the final stage. For
> + *       example with a query such as:
>
> I'd omit this hunk.  The serialStates thing is really separate from
> what's being talked about here, and you discuss it further down.

Removed.

I've attached 2 of the patches which are affected by the changes.

Thanks for looking over this.

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