Horton

Re: [HACKERS] Partition-wise aggregation/grouping

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>

View thread View on postgr.es
From: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
To: Jeevan Chalke <jeevan.chalke@enterprisedb.com>
Cc: Robert Haas <robertmhaas@gmail.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-15T10:08:51Z
Lists: pgsql-hackers

Attachments

On Wed, Mar 14, 2018 at 7:51 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
>
>
> On Tue, Mar 13, 2018 at 7:43 PM, Jeevan Chalke
> <jeevan.chalke@enterprisedb.com> wrote:
>>
>> Hi,
>>
>> The patch-set is complete now. But still, there is a scope of some comment
>> improvements due to all these refactorings. I will work on it. Also, need to
>> update few documentations and indentations etc. Will post those changes in
>> next patch-set. But meanwhile, this patch-set is ready to review.
>
>
> Attached complete patch-set.
>
> Fixed all remaining documentation, indentation and comments.
> Also incorporated few comments improvements provided off-list by Ashutosh
> Bapat.
>

The patchset needs rebase.  I have rebased those on the latest head
and made following changes.

Argument force_partial_agg is added after output arguments to
make_grouping_rels(). Moved it before output arguemnts to keep input and output
arguments separate.

Also moved the comment about creating partial aggregate paths before full
aggregate paths in make_grouping_rels() moved to
populate_grouping_rels_with_paths().

In create_grouping_paths() moved call to try_degenerate_grouping_paths() before
computing extra grouping information.

Some more comment changes in the attached patch set.

+         *
+         * With partitionwise aggregates, we may have childrel's pathlist empty
+         * if we are doing partial aggregation. Thus do this only if childrel's
+         * pathlist is not NIL.
          */
-        if (childrel->cheapest_total_path->param_info == NULL)
+        if (childrel->pathlist != NIL &&
+            childrel->cheapest_total_path->param_info == NULL)
             accumulate_append_subpath(childrel->cheapest_total_path,
                                       &subpaths, NULL);
I thought we got rid of this code. Why has it come back? May be the comment
should point to a function where this case happen.

In populate_grouping_rels_with_paths(), we need to pass extra to
extension hook create_upper_paths_hook similar to what
add_paths_to_joinrel() does.

Also, we aren't passing is_partial_agg to FDW hook, so it won't know
whether to compute partial or full aggregates. I think the check
5296         /* Partial aggregates are not supported. */
5297         if (extra->partial_partitionwise_grouping)
5298             return;
in add_foreign_grouping_paths() is wrong. It's checking whether the
children of the given relation will produce partial aggregates or not.
But it is supposed to check whether the given relation should produce
partial aggregates or not. I think we need to include is_partial_agg
in GroupPathExtraData so that it gets passed to FDWs. If we do so, we
need to add a comment in the prologue of GroupPathExtraData to
disambiguate usage of is_partial_agg and
partial_partitionwise_grouping.

In current create_grouping_paths() (without any of your patches
applied) we first create partial paths in partially grouped rel and
then add parallel path to grouped rel using those partial paths. Then
we hand over this to FDW and extension hooks, which may add partial
paths, which might throw away a partial path used to create a parallel
path in grouped rel causing a segfault. I think this bug exists since
we introduced parallel aggregation or upper relation refactoring
whichever happened later. Introduction of partially grouped rel has
just made it visible.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database 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.