On Thu, Mar 22, 2018 at 10:06 AM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote:
> On Thu, Mar 22, 2018 at 3:26 AM, Robert Haas <robertmh...@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=gp9b...@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
