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