On Tue, Jan 19, 2016 at 4:50 PM, David Rowley <david.rowley@2ndquadrant.com> wrote: >> Oh, one more point: is there any reason why all of this needs to be a >> single (giant) patch? I feel like the handling of internal states >> could be a separate patch from the basic patch to allow partial >> aggregation and aggregate-combining, and that the latter could be >> committed first if we had a reasonably final version of it. That >> seems like it would be easier from a review standpoint, and might >> allow more development to proceed in parallel, too. > > I didn't ever really imagine that I'd include any actual new combinefns or > serialfn/deserialfn in this patch. One set has of course now ended up in > there, I can move these off to the test patch for now. > > Did you imagine that the first patch in the series would only add the > aggcombinefn column to pg_aggregate and leave the aggserialfn stuff until > later?
I think that would be a sensible way to proceed. > I thought it seemed better to get the infrastructure committed in one > hit, then add a bunch of new combinefn, serialfn, deserialfns for existing > aggregates in follow-on commits.
To my mind, priority #1 ought to be putting this fine new functionality to some use. Expanding it to every aggregate we've got seems like a distinctly second priority. That's not to say that it's absolutely gotta go down that way, but those would be my priorities.
Agreed. So I've attached a version of the patch which does not have any of the serialise/deserialise stuff in it.
I've also attached a test patch which modifies the grouping planner to add a Partial Aggregate node, and a final aggregate node when it's possible. Running the regression tests with this patch only shows up variances in the EXPLAIN outputs, which is of course expected.
