Re: Combining Aggregates

From: David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Amit Kapila <amit(dot)kapila(at)enterprisedb(dot)com>
Subject: Re: Combining Aggregates
Date: 2016-01-20 12:38:39
Message-ID: CAKJS1f_voy=y=HjnBNSgwozQbsQzFheJG-Hya1P0vb1zS86Jig@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 20 January 2016 at 10:54, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Tue, Jan 19, 2016 at 4:50 PM, David Rowley
> <david(dot)rowley(at)2ndquadrant(dot)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.

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

Attachment Content-Type Size
combine_aggregate_state_d6d480b_2016-01-21.patch application/octet-stream 84.5 KB
combine_aggs_test_v6.patch application/octet-stream 9.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2016-01-20 12:44:42 Re: Combining Aggregates
Previous Message Michael Paquier 2016-01-20 11:54:26 Re: dynloader.h missing in prebuilt package for Windows?