Re: Parallel Aggregate

On Sun, Mar 13, 2016 at 5:44 PM, David Rowley
<david(dot)rowley(at)2ndquadrant(dot)com> wrote:
> On 12 March 2016 at 16:31, David Rowley <david(dot)rowley(at)2ndquadrant(dot)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 haven't fully studied every line of this yet, but here are a few comments:

+ case T_PartialAggref:
+ coll = InvalidOid; /* XXX is this correct? */
+ break;

I doubt it. More generally, why are we inventing PartialAggref
instead of reusing Aggref? The code comments seem to contain no
indication as to why we shouldn't need all the same details for
PartialAggref that we do for Aggref, instead of only a small subset of
them. Hmm... actually, it looks like PartialAggref is intended to
wrap Aggref, but that seems like a weird design. Why not make Aggref
itself DTRT? There's not enough explanation in the patch of what is
going on here and why.

}
+ if (can_parallel)
+ {

Seems like a blank line would be in order.

I don't see where this applies has_parallel_hazard or anything
comparable to the aggregate functions. I think it needs to do that.

+ /* XXX +1 ? do we expect the main process to actually do real work? */
+ numPartialGroups = Min(numGroups, subpath->rows) *
+ (subpath->parallel_degree + 1);

I'd leave out the + 1, but I don't think it matters much.

+ aggstate->finalizeAggs == true)

We usually just say if (a) not if (a == true) when it's a boolean.
Similarly !a rather than a == false.

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