Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 22, 2018 at 10:06 AM, Ashutosh Bapat <
ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:

> On Thu, Mar 22, 2018 at 3:26 AM, Robert Haas <robertmhaas(at)gmail(dot)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