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: Robert Haas <robertmhaas@gmail.com>
Cc: Jeevan Chalke <jeevan.chalke@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-16T16:45:52Z
Lists: pgsql-hackers 
On Thu, Mar 15, 2018 at 7:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Mar 15, 2018 at 6:08 AM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>> 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.
>
> I don't think there's really a problem here; or at least if there is,
> I'm not seeing it.  If an FDW wants to introduce partially grouped
> paths, it should do so when it is called for
> UPPERREL_PARTIAL_GROUP_AGG from within
> add_paths_to_partial_grouping_rel.  If it wants to introduce fully
> grouped paths, it should do so when it is called for
> UPPERREL_GROUP_AGG from within create_grouping_paths.  By the time the
> latter call is made, it's too late to add partially grouped paths; if
> the FDW does, that's a bug in the FDW.

Right.

>
> Admittedly, this means that commit
> 3bf05e096b9f8375e640c5d7996aa57efd7f240c subtly changed the API
> contract for FDWs.  Before that, an FDW that wanted to support partial
> aggregation would have needed to add partially grouped paths to
> UPPERREL_GROUP_AGG when called for that relation; whereas now it would
> need to add them to UPPERREL_PARTIAL_GROUP_AGG when called for that
> relation.

And when an FDW added partial paths to the grouped rel
(UPPERREL_GROUP_AGG) it might throw away the partial paths gathered by
the core code. 3bf05e096b9f8375e640c5d7996aa57efd7f240c has fixed that
bug by allowing FDW to add partial paths to UPPERREL_PARTIAL_GROUP_AGG
before adding gather paths to the UPPERREL_GROUP_AGG. But I agree that
it has subtly changed that API and we need to add this to the
documentation (may be we should have added that in the commit which
introduced partially grouped relation).

> This doesn't actually falsify any documentation, though,
> because this oddity wasn't documented before.  Possibly more
> documentation could stand to be written in this area, but that's not
> the topic of this thread.

+1.

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