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.