Re: Parallel Aggregate costs don't consider combine/serial/deserial funcs

On Sun, Apr 10, 2016 at 8:47 AM, David Rowley
<david(dot)rowley(at)2ndquadrant(dot)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