Horton

Re: [HACKERS] Partition-wise aggregation/grouping

Robert Haas <robertmhaas@gmail.com>

View thread View on postgr.es
From: Robert Haas <robertmhaas@gmail.com>
To: Jeevan Chalke <jeevan.chalke@enterprisedb.com>
Cc: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>, Dilip Kumar <dilipbalaut@gmail.com>, David Rowley <david.rowley@2ndquadrant.com>, Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2018-01-15T22:11:42Z
Lists: pgsql-hackers 
On Thu, Jan 11, 2018 at 6:00 AM, Jeevan Chalke
<jeevan.chalke@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

Commits

  1. postgres_fdw: Push down partition-wise aggregation.

  2. Remove 'target' from GroupPathExtraData.

  3. Implement partition-wise grouping/aggregation.

  4. Don't pass the grouping target around unnecessarily.

  5. Determine grouping strategies in create_grouping_paths.

  6. Defer creation of partially-grouped relation until it's needed.

  7. Split create_grouping_paths into degenerate and non-degenerate cases.

  8. Pass additional arguments to a couple of grouping-related functions.

  9. Fix logic error in add_paths_to_partial_grouping_rel.

  10. Minor cleanup of code related to partially_grouped_rel.

  11. Add a new upper planner relation for partially-aggregated results.

  12. Charge cpu_tuple_cost * 0.5 for Append and MergeAppend nodes.

  13. Rename enable_partition_wise_join to enable_partitionwise_join

  14. Factor some code out of create_grouping_paths.

  15. Pad XLogReaderState's main_data buffer more aggressively.

  16. Prevent int128 from requiring more than MAXALIGN alignment.

  17. Fix DROP SUBSCRIPTION hang

  18. Inject $(ICU_LIBS) regardless of platform.

  19. Some preliminary refactoring towards partitionwise join.