Re: Parallel Aggregate - Mailing list pgsql-hackers

I read this a bit, as an exercise to try to follow parallel query a bit.
I think the most interesting thing I have to say is that the new error
messages in ExecInitExpr do not conform to our style.  Probably just
downcase everything except Aggref and you're done, since they're
can't-happen conditions anyway.  The comments below are mostly as I
learn how the whole thing works so if I'm talking nonsense, I'm happy to
be ignored or, better yet, educated.

I think the way we set the "consider_parallel" flag is a bit odd (we
just "return" out of the function in the cases were it mustn't be set);
but that mechanism is already part of set_rel_consider_parallel and
similar to (but not quite like) longstanding routines such as
set_rel_width, so nothing new in this patch.  I find this a bit funny
coding, but then this is the planner so maybe it's okay.

I think the comment on search_indexed_tlist_for_partial_aggref is a bit
bogus; it says it returns an existing Var, but what it does is
manufacture one itself.  I *think* the code is all right, but the
comment seems misleading.

In set_combineagg_references(), there are two calls to
fix_combine_agg_expr(); I think the one hanging on the
search_indexed_tlist_for_sortgroupref call is useless; you could just
have the "if newexpr != NULL" in the outer block (after initializing to
NULL before the ressortgroupref check).

set_combineagg_references's comment says it does something "similar to
set_upper_references, and additionally" some more stuff, but reading the
code for both functions I think that's not quite true.  I think the
comment should say that both functions are parallel, but one works for
partial aggs and the other doesn't.  Actually, what happens if you feed
an agg plan with combineStates to set_upper_references?  If it still
works but the result is not optimal, maybe we should check against that
case, so as to avoid the case where somebody hacks this further and the
plans are suddenly not agg-combined anymore.  What I actually expect to
happen is that something would explode during execution; in that case
perhaps it's better to add a comment?  (In further looking at other
setrefs.c similar functions, maybe it's fine the way you have it.)

Back at make_partialgroup_input_target, the comment says "so that they
can be found later in Combine Aggregate nodes during setrefs".  I think
it's better to be explicit and say ".. can be found later during
set_combineagg_references" or something.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services