Horton

Re: [HACKERS] Partition-wise aggregation/grouping

Jeevan Chalke <jeevan.chalke@enterprisedb.com>

View thread View on postgr.es
From: Jeevan Chalke <jeevan.chalke@enterprisedb.com>
To: Ashutosh Bapat <ashutosh.bapat@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-22T10:27:31Z
Lists: pgsql-hackers 
On Thu, Mar 22, 2018 at 10:06 AM, Ashutosh Bapat <
ashutosh.bapat@enterprisedb.com> wrote:

> On Thu, Mar 22, 2018 at 3:26 AM, Robert Haas <robertmhaas@gmail.com>
> wrote:
> >
> > Is there a good reason not to use input_rel->relids as the input to
> > fetch_upper_rel() in all cases, rather than just at subordinate
> > levels?
> >
>
> That would simplify some code in these patches. We have set
> upper_rel->relids to NULL for non-other upper relation since Tom
> expected to use relids to mean something other than scan/join relids.
> With these patch-sets for grouping rels we are using upper_rel->relids
> to the relids of underlying scan/join relation. So it does make sense
> to set relids to input_rel->relids for all the grouping rels whether
> "other" or non-"other" grouping rels.
>
> But with this change, we have to change all the existing code to pass
> input_rel->relids to fetch_upper_rel(). If we don't do that or in
> future somebody calls that function with relids = NULL we will produce
> two relations which are supposed to do the same thing but have
> different relids set. That's because fetch_upper_rel() creates a
> relation if one does not exist whether or not the caller intends to
> create one. We should probably create two functions 1. to build an
> upper relation and 2. to search for it similar to what we have done
> for join relations and base relation. The other possibility is to pass
> a flag to fetch_upper_rel() indicating whether a caller intends to
> create a new relation when one doesn't exist. With this design a
> caller can be sure that an upper relation will not be created when it
> wants to just fetch an existing relation (and error out/assert if it
> doesn't find one.).
>

Like Ashutosh said, splitting fetch_upper_rel() in two functions,
build_upper_rel() and find_upper_rel() looks better.

However, I am not sure whether setting relids in a top-most grouped rel is
a good idea or not. I remember we need this while working on Aggregate
PushDown, and in [1] Tom Lane opposed the idea of setting the relids in
grouped_rel.

If we want to go with this, then I think it should be done as a separate
stand-alone patch.

[1]
https://www.postgresql.org/message-id/CAFjFpRdUz6h6cmFZFYAngmQAX8Zvo+MZsPXidZ077h=gp9bvQw@mail.gmail.com


>
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company
>



-- 
Jeevan Chalke
Technical Architect, Product Development
EnterpriseDB Corporation
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.