Re: Parallel Aggregate

Hi,

On 03/13/2016 10:44 PM, David Rowley wrote:
> On 12 March 2016 at 16:31, David Rowley <david.rowley@2ndquadrant.com> wrote:
>> I've attached an updated patch which is based on commit 7087166,
>> things are really changing fast in the grouping path area at the
>> moment, but hopefully the dust is starting to settle now.
>
> The attached patch fixes a harmless compiler warning about a
> possible uninitialised variable.

I've looked at this patch today. The patch seems quite solid, but I do 
have a few minor comments (or perhaps questions, given that this is the 
first time I looked at the patch).

1) exprCollation contains this bit:
-----------------------------------
    case T_PartialAggref:        coll = InvalidOid; /* XXX is this correct? */        break;

I doubt this is the right thing to do. Can we actually get to this piece 
of code? I haven't tried too hard, but regression tests don't seem to 
trigger this piece of code.

Moreover, if we're handling PartialAggref in exprCollation(), maybe we 
should handle it also in exprInputCollation and exprSetCollation?

And if we really need the collation there, why not to fetch the actual 
collation from the nested Aggref? Why should it be InvalidOid?


2) partial_aggregate_walker
---------------------------

I think this should follow the naming convention that clearly identifies 
the purpose of the walker, not what kind of nodes it is supposed to 
walk. So it should be:
    aggregates_allow_partial_walker


3) create_parallelagg_path
--------------------------

I do agree with the logic that under-estimates are more dangerous than 
over-estimates, so the current estimate is safer. But I think this would 
be a good place to apply the formula I proposed a few days ago (or 
rather the one Dean Rasheed proposed in response).

That is, we do know that there are numGroups in total, and each parallel 
worker sees subpath->rows then it's expected to see
    sel = (subpath->rows / rel->tuples);    perGroup = (rel->tuples / numGroups);    workerGroups = numGroups * (1 -
powl(1- s, perGroup));    numPartialGroups = numWorkers * workerGroups
 

It's probably better to see Dean's message from 13/3.

4) Is clauses.h the right place for PartialAggType?

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services