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.
Oops. > 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 happens. > 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: http://www.postgresql.org/mailpref/pgsql-hackers
