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: Ashutosh Bapat <ashutosh.bapat@enterprisedb.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-15T14:16:48Z
Lists: pgsql-hackers 
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.

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

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