Re: [HACKERS] Rethinking representation of partial-aggregate steps

2016-06-27 Thread Tom Lane
David Rowley writes: > I can't help wonder how plan to allow future expansions of > non-serialized partial aggregates giving that in setrefs.c you're > making a hard assumption that mark_partial_aggref() should always > receive the SERIAL versions of the aggsplit.

Re: [HACKERS] Rethinking representation of partial-aggregate steps

2016-06-26 Thread David Rowley
On 27 June 2016 at 03:36, Tom Lane wrote: > After having worked on the patch some, I think that AggSplit is a pretty > good choice, because it is about how we split up the calculation of an > aggregate. And it's short, which is a good thing for something that's > going to be

Re: [HACKERS] Rethinking representation of partial-aggregate steps

2016-06-26 Thread David Rowley
On 27 June 2016 at 08:28, Tom Lane wrote: > David Rowley writes: >> On 27 June 2016 at 03:36, Tom Lane wrote: >>> Looking at this in the light of morning, I'm rather strongly tempted to >>> invert the sense of the FINALIZE

Re: [HACKERS] Rethinking representation of partial-aggregate steps

2016-06-26 Thread Tom Lane
I wrote: > [ shrug... ] I do not buy that argument, because it doesn't justify > the COMBINE option: why shouldn't that be inverted, ie USEFINALFUNC? Sorry, I meant USETRANSFUNC of course. regards, tom lane -- Sent via pgsql-hackers mailing list

Re: [HACKERS] Rethinking representation of partial-aggregate steps

2016-06-26 Thread Tom Lane
David Rowley writes: > On 27 June 2016 at 03:36, Tom Lane wrote: >> Looking at this in the light of morning, I'm rather strongly tempted to >> invert the sense of the FINALIZE option, so that "simple" mode works out >> as zero, ie, select no

Re: [HACKERS] Rethinking representation of partial-aggregate steps

2016-06-26 Thread David Rowley
On 27 June 2016 at 03:36, Tom Lane wrote: > Looking at this in the light of morning, I'm rather strongly tempted to > invert the sense of the FINALIZE option, so that "simple" mode works out > as zero, ie, select no options. Maybe call it SKIPFINAL instead of > FINALIZE?

Re: [HACKERS] Rethinking representation of partial-aggregate steps

2016-06-26 Thread Tom Lane
David Rowley writes: > On 26 June 2016 at 04:07, Tom Lane wrote: >> After a bit of thought, maybe AggDivision or AggSplit or something >> along those lines? > How about AggCompletion? It's seems to fit well in the sense of the > aggregation

Re: [HACKERS] Rethinking representation of partial-aggregate steps

2016-06-26 Thread David Rowley
On 26 June 2016 at 04:07, Tom Lane wrote: > I wrote: >> David Rowley writes: >>> The attached implements this, with the exception that I didn't really >>> think AggPartialMode was the best name for the enum. I've named this >>> AggregateMode

Re: [HACKERS] Rethinking representation of partial-aggregate steps

2016-06-25 Thread Tom Lane
I wrote: > David Rowley writes: >> The attached implements this, with the exception that I didn't really >> think AggPartialMode was the best name for the enum. I've named this >> AggregateMode instead, as the aggregate is only partial in some cases. > Hm. We

Re: [HACKERS] Rethinking representation of partial-aggregate steps

2016-06-25 Thread Tom Lane
David Rowley writes: > The attached implements this, with the exception that I didn't really > think AggPartialMode was the best name for the enum. I've named this > AggregateMode instead, as the aggregate is only partial in some cases. Hm. We already have an

Re: [HACKERS] Rethinking representation of partial-aggregate steps

2016-06-23 Thread David Rowley
On 24 June 2016 at 05:55, Tom Lane wrote: > #define AGGOP_COMBINESTATES 0x1 > #define AGGOP_SERIALIZESTATES 0x2 > #define AGGOP_DESERIALIZESTATES 0x4 > #define AGGOP_FINALIZEAGGS 0x8 > > typedef enum AggPartialMode > { > AGGPARTIAL_SIMPLE = AGGOP_FINALIZEAGGS, >

Re: [HACKERS] Rethinking representation of partial-aggregate steps

2016-06-23 Thread Tom Lane
David Rowley writes: > As for the above proposal, I do agree that it'll be cleaner with bit > flags, I just really don't see the need for the AGGTYPE_* alias > macros. For me it's easier to read if each option is explicitly listed > similar to how pull_var_clause()

Re: [HACKERS] Rethinking representation of partial-aggregate steps

2016-06-23 Thread David Rowley
On 24 June 2016 at 05:12, Robert Haas wrote: > I do agree, however, that the three Boolean flags don't make the code > entirely easy to read. What I might suggest is that we replace the > three Boolean flags with integer flags, something like this: > > #define

Re: [HACKERS] Rethinking representation of partial-aggregate steps

2016-06-23 Thread David Rowley
On 24 June 2016 at 05:25, Tom Lane wrote: > Robert Haas writes: >> Hmm, well I guess I would have to disagree with the idea that those >> other modes aren't useful. I seem to recall that David had some >> performance results showing that pushing partial

Re: [HACKERS] Rethinking representation of partial-aggregate steps

2016-06-23 Thread Tom Lane
Alvaro Herrera writes: > Tom Lane wrote: >> Yeah, that's another way we could go. I had been considering a variant >> of that, which was to assign specific code values to the enum constants >> and then invent macros that did bit-anding tests on them. That ends up >>

Re: [HACKERS] Rethinking representation of partial-aggregate steps

2016-06-23 Thread Alvaro Herrera
Tom Lane wrote: > What I'm imagining is, say, > > #define AGGOP_COMBINESTATES 0x1 > #define AGGOP_SERIALIZESTATES 0x2 > #define AGGOP_DESERIALIZESTATES 0x4 > #define AGGOP_FINALIZEAGGS 0x8 > > typedef enum AggPartialMode > { > AGGPARTIAL_SIMPLE = AGGOP_FINALIZEAGGS, > AGGPARTIAL_PARTIAL

Re: [HACKERS] Rethinking representation of partial-aggregate steps

2016-06-23 Thread Simon Riggs
On 23 June 2016 at 18:31, Robert Haas wrote: > > Sure, but aggregating as early as possible will often have the effect > of dramatically reducing the number of tuples that have to pass > through upper levels of the plan tree, which seems it would frequently > far outweigh

Re: [HACKERS] Rethinking representation of partial-aggregate steps

2016-06-23 Thread Alvaro Herrera
Tom Lane wrote: > Robert Haas writes: > > I do agree, however, that the three Boolean flags don't make the code > > entirely easy to read. What I might suggest is that we replace the > > three Boolean flags with integer flags, something like this: > > Yeah, that's

Re: [HACKERS] Rethinking representation of partial-aggregate steps

2016-06-23 Thread Robert Haas
On Thu, Jun 23, 2016 at 1:25 PM, Tom Lane wrote: > Robert Haas writes: >> On Thu, Jun 23, 2016 at 12:26 PM, Tom Lane wrote: >>> I'm inclined to think that we should aggressively simplify here, perhaps >>> replacing all three of

Re: [HACKERS] Rethinking representation of partial-aggregate steps

2016-06-23 Thread Tom Lane
Robert Haas writes: > On Thu, Jun 23, 2016 at 12:26 PM, Tom Lane wrote: >> I'm inclined to think that we should aggressively simplify here, perhaps >> replacing all three of these flags with a three-way enum, say >> >> PARTIALAGG_NORMAL /* simple

Re: [HACKERS] Rethinking representation of partial-aggregate steps

2016-06-23 Thread Robert Haas
On Thu, Jun 23, 2016 at 12:26 PM, Tom Lane wrote: > I really dislike this aspect of the partial-aggregate patch: > > typedef struct Agg > { > ... > boolcombineStates; /* input tuples contain transition states */ > boolfinalizeAggs; /* should we

[HACKERS] Rethinking representation of partial-aggregate steps

2016-06-23 Thread Tom Lane
I really dislike this aspect of the partial-aggregate patch: typedef struct Agg { ... boolcombineStates; /* input tuples contain transition states */ boolfinalizeAggs; /* should we call the finalfn on agg states? */ boolserialStates; /* should agg