On Tue, Apr 12, 2016 at 5:38 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
>>> 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?
>
> That seems right, but I just can't think of how its possible to
> parallelise these aggregates anyway.
Well, if you could ensure that each worker would see a whole group,
you could do it, I think. But it's probably fine to just leave this
for now. It's not like it can't be changed if somebody figures out
some cool thing to do in this area.
>>> 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.
>
> hmm, I'm not sure how it's subtly different. Do you mean the
> preference towards costing the finalfn when finalizeAggs is true, and
> ignoring the serialfn in this case? nodeAgg.c should do the same,
> although it'll deserialize in such a case. We can never finalize and
> serialize in the same node.
I mean that, IIUC, in some other places where you use serialStates,
true means that (de)serialization is known to be needed. Here,
however, it only means it might be needed, contingent on whether the
serial/deserial functions are actually present.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
