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-13T03:42:49Z
Lists: pgsql-hackers 
On Mon, Mar 12, 2018 at 7:49 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
>
>
> On Mon, Mar 12, 2018 at 6:07 PM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>>
>> On Fri, Mar 9, 2018 at 4:21 PM, Ashutosh Bapat
>>
>> GroupPathExtraData now completely absorbs members from and replaces
>> OtherUpperPathExtraData. This means that we have to consider a way to
>> pass GroupPathExtraData to FDWs through GetForeignUpperPaths(). I
>> haven't tried it in this patch.
>
>
> Initially, I was passing OtherUpperPathExtraData to FDW. But now we need to
> pass GroupPathExtraData.
>
> However, since GetForeignUpperPaths() is a generic function for all upper
> relations, we might think of renaming this struct to UpperPathExtraData. Add
> an UpperRelationKind member to it
> Which will be used to distinguish the passed in extra data. But now we only
> have extra data for grouping only, I chose not to do that here. But someone,
> when needed, may choose this approach.

We don't need UpperRelationKind member in that structure. That will be
provided by the RelOptInfo passed.

The problem here is the extra information required for grouping is not
going to be the same for that needed for window aggregate and
certainly not for ordering. If we try to jam everything in the same
structure, it will become large with many members useless for a given
operation. A reader will not have an idea about which of them are
useful and which of them are not. So, instead we should try some
polymorphism. I think we can pass a void * to GetForeignUpperPaths()
and corresponding FDW hook knows what to cast it to based on the
UpperRelationKind passed.

BTW, the patch has added an argument to GetForeignUpperPaths() but has
not documented the change in API. If we go the route of polymorphism,
we will need to document the mapping between UpperRelationKind and the
type of structure passed in.

>
>>
>> With this patch there's a failure in partition_aggregation where the
>> patch is creating paths with MergeAppend with GatherMerge underneath.
>> I think this is related to the call
>> add_paths_to_partial_grouping_rel() when try_parallel_aggregation is
>> true. But I didn't investigate it further.
>
>
> I fixed it. We need to pass  is_partial_agg instead of
> extra->partial_partitionwise_grouping while calling
> add_paths_to_partial_grouping_rel() in case of parallelism.

Thanks for investigation and the fix.

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