Re: Rethinking representation of partial-aggregate steps

On Thu, Jun 23, 2016 at 1:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Thu, Jun 23, 2016 at 12:26 PM, Tom Lane <tgl@sss.pgh.pa.us> 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 aggregation */
>>> PARTIALAGG_PARTIAL      /* lower phase of partial aggregation */
>>> PARTIALAGG_FINAL        /* upper phase of partial aggregation */
>>>
>>> This embodies a couple of judgments that maybe someone would quibble with:
>>> * we don't have any use for intermediate partial aggregation steps;
>>> * we don't have any use for partial aggregation in which data transferred
>>> up needn't be serialized.
>
>> 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 aggregate steps below
>> an Append node resulted in a noticeable speed-up even in the absence
>> of any parallelism, presumably because it avoids whatever projection
>> the Append might do, and also improves icache and dcache locality.
>
> I don't believe that for one second, because introducing another layer of
> intermediate aggregation implies another projection step, plus all the
> other overhead of a Plan node.

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 those considerations.  Anyway, you can go back and find
what he posted about this previously and look at it for yourself;
that's probably better than having an argument with me about his test
results.

> Also, if we are going to support intermediate partial aggregation, the
> current representation is broken anyway, since it lacks any ability
> to distinguish serialization for the input states from serialization
> for the output states, which is something you'd certainly need to
> distinguish in this type of plan structure.

I mentioned that in the very same email to which you were replying
when you wrote this, which makes me wonder whether you bothered to
read the whole thing.

>> 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 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
> being just about what you propose except that the compiler understands
> the enum-ness of the behavioral alternatives, which seems like a good
> thing.

I don't object to that.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company