On Thu, Jan 11, 2018 at 6:00 AM, Jeevan Chalke <jeevan.cha...@enterprisedb.com> wrote: > Attached new set of patches adding this. Only patch 0007 (main patch) and > 0008 (testcase patch) has changed. > > Please have a look and let me know if I missed any.
I spent a little time studying 0001 and 0002 today, as well as their relation with 0007. I find the way that the refactoring has been done slightly odd. With 0001 and 0002 applied, we end up with three functions for creating aggregate paths: create_partial_agg_path, which handles the partial-path case for both sort and hash; create_sort_agg_path, which handles the sort case for non-partial paths only; and create_hash_agg_path, which handles the hash case for non-partial paths only. This leads to the following code in 0007: + /* Full grouping paths */ + + if (try_parallel_aggregation) + { + Assert(extra->agg_partial_costs && extra->agg_final_costs); + create_partial_agg_path(root, input_rel, grouped_rel, target, + partial_target, extra->agg_partial_costs, + extra->agg_final_costs, gd, can_sort, + can_hash, (List *) extra->havingQual); + } + + if (can_sort) + create_sort_agg_path(root, input_rel, grouped_rel, target, + partial_target, agg_costs, + extra->agg_final_costs, gd, can_hash, + dNumGroups, (List *) extra->havingQual); + + if (can_hash) + create_hash_agg_path(root, input_rel, grouped_rel, target, + partial_target, agg_costs, + extra->agg_final_costs, gd, dNumGroups, + (List *) extra->havingQual); That looks strange -- you would expect to see either "sort" and "hash" cases here, or maybe "partial" and "non-partial", or maybe all four combinations, but seeing three things here looks surprising. I think the solution is just to create a single function that does both the work of create_sort_agg_path and the work of create_hash_agg_path instead of having two separate functions. A related thing that is also surprising is that 0007 manages to reuse create_partial_agg_path for both the isPartialAgg and non-isPartialAgg cases -- in fact, the calls appear to be identical, and could be hoisted out of the "if" statement -- but create_sort_agg_path and create_hash_agg_path do not get reused. I think you should see whether you can define the new combo function that can be used for both cases. The logic looks very similar, and I'm wondering why it isn't more similar than it is; for instance, create_sort_agg_path loops over the input rel's pathlist, but the code for isPartialAgg/can_sort seems to consider only the cheapest path. If this is correct, it needs a comment explaining it, but I don't see why it should be correct. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company