On Sun, Apr 10, 2016 at 8:47 AM, David Rowley
<david.row...@2ndquadrant.com> wrote:
> I realised a few days ago that the parallel aggregate code does not
> cost for the combine, serialisation and deserialisation functions at
> all.


> I've attached a patch which fixes this.

I've committed this patch.  I wonder if it's going to produce compiler
warnings for some people, complaining about possible use of an
uninitialized variable.  That would kind of suck.  I don't much mind
having to insert a dummy assignment to shut the compiler up; a smarter
compiler will just throw it out anyway.  I'm less enthused about a
dummy MemSet.  The compiler is less likely to be able to get rid of
that, and it's more expensive if it doesn't.  But let's see what

> One small point which I was a little unsure of in the attached is,
> should the "if (aggref->aggdirectargs)" part of
> count_agg_clauses_walker() be within the "if
> (!context->combineStates)". I simply couldn't decide. We currently
> have no aggregates which this affects anyway, per; select * from
> pg_aggregate where aggcombinefn <> 0 and aggkind <> 'n'; so for now
> I've left it outwith.

The direct arguments would be evaluated in the worker, but not in the
leader, right?  Or am I confused?

> Another thing I thought of is that it's not too nice that I have to
> pass 3 bools to count_agg_clauses() in order to tell it what to do. I
> was tempted to invent some bitmask flags for this, then modify
> create_agg_path() to use the same flags, but I thought I'd better not
> cause too much churn with this patch.

I'm kinda tempted to say this should be using an enum.  I note that
serialStates has a subtly different meaning here than in some other
places where you have used the same term.

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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to