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-03-05T20:59:36Z
Lists: pgsql-hackers 
On Mon, Mar 5, 2018 at 3:56 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
> However, to perform Gather or Gather Merge once we have all partial paths
> ready, and to avoid too many existing code rearrangement, I am calling
> try_partitionwise_grouping() before we do any aggregation/grouping on whole
> relation. By doing this, we will be having all partial paths in
> partially_grouped_rel and then existing code will do required finalization
> along with any Gather or Gather Merge, if required.
>
> Please have a look over attached patch-set and let me know if it needs
> further changes.

This does look better.

+      <term><varname>enable_partitionwise_agg</varname> (<type>boolean</type>)

Please don't abbreviate "aggregate" to "agg".

-       /* Build final grouping paths */
-       add_paths_to_grouping_rel(root, input_rel, grouped_rel, target,
-
partially_grouped_rel, agg_costs,
-
&agg_final_costs, gd, can_sort, can_hash,
-                                                         dNumGroups,
(List *) parse->havingQual);
+       if (isPartialAgg)
+       {
+               Assert(agg_partial_costs && agg_final_costs);
+               add_paths_to_partial_grouping_rel(root, input_rel,
+
           partially_grouped_rel,
+
           agg_partial_costs,
+
           gd, can_sort, can_hash,
+
           false, true);
+       }
+       else
+       {
+               double          dNumGroups;
+
+               /* Estimate number of groups. */
+               dNumGroups = get_number_of_groups(root,
+
           cheapest_path->rows,
+
           gd,
+
           child_data ? make_tlist_from_pathtarget(target) :
parse->targetList);
+
+               /* Build final grouping paths */
+               add_paths_to_grouping_rel(root, input_rel, grouped_rel, target,
+
partially_grouped_rel, agg_costs,
+
agg_final_costs, gd, can_sort, can_hash,
+
dNumGroups, (List *) havingQual);
+       }

This looks strange.  Why do we go down two completely different code
paths here?  It seems to me that the set of paths we add to the
partial_pathlist shouldn't depend at all on isPartialAgg.  I might be
confused, but it seems to me that any aggregate path we construct that
is going to run in parallel must necessarily be partial, because even
if each group will occur only in one partition, it might still occur
in multiple workers for that partition, so finalization would be
needed.  On the other hand, for non-partial paths, we can add then to
partially_grouped_rel when isPartialAgg = true and to grouped_rel when
isPartialAgg = false, with the only difference being AGGSPLIT_SIMPLE
vs. AGGSPLIT_INITIAL_SERIAL.  But that doesn't appear to be what this
is doing.

+       /*
+        * If there are any fully aggregated partial paths present,
may be because
+        * of parallel Append over partitionwise aggregates, we must stick a
+        * Gather or Gather Merge path atop the cheapest partial path.
+        */
+       if (grouped_rel->partial_pathlist)

This comment is copied from someplace where the code does what the
comment says, but here it doesn't do any such thing.

More tomorrow...

-- 
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.