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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
