On Mon, Mar 12, 2018 at 6:07 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote:
> On Fri, Mar 9, 2018 at 4:21 PM, Ashutosh Bapat > <ashutosh.ba...@enterprisedb.com> wrote: > > On Thu, Mar 8, 2018 at 7:31 PM, Robert Haas <robertmh...@gmail.com> > wrote: > >> > >> This kind of goes along with the suggestion I made yesterday to > >> introduce a new function, which at the time I proposed calling > >> initialize_grouping_rel(), to set up new grouped or partially grouped > >> relations. By doing that it would be easier to ensure the > >> initialization is always done in a consistent way but only for the > >> relations we actually need. But maybe we should call it > >> fetch_grouping_rel() instead. The idea would be that instead of > >> calling fetch_upper_rel() we would call fetch_grouping_rel() when it > >> is a question of the grouped or partially grouped relation. It would > >> either return the existing relation or initialize a new one for us. I > >> think that would make it fairly easy to initialize only the ones we're > >> going to need. > > > > Hmm. I am working on refactoring the code to do something like this. > > > > Here's patch doing the same. I have split create_grouping_paths() into > three functions 1. to handle degenerate grouping paths > (try_degenerate_grouping_paths()) 2. to create the grouping rels, > partial grouped rel and grouped rel (make_grouping_rels()), which also > sets some properties in GroupPathExtraData. 3. populate grouping rels > with paths (populate_grouping_rels_with_paths()). With those changes, > I have been able to get rid of partially grouped rels when they are > not necessary. But I haven't tried to get rid of grouped_rels when > they are not needed. > > GroupPathExtraData now completely absorbs members from and replaces > OtherUpperPathExtraData. This means that we have to consider a way to > pass GroupPathExtraData to FDWs through GetForeignUpperPaths(). I > haven't tried it in this patch. > > With this patch there's a failure in partition_aggregation where the > patch is creating paths with MergeAppend with GatherMerge underneath. > I think this is related to the call > add_paths_to_partial_grouping_rel() when try_parallel_aggregation is > true. But I didn't investigate it further. > > With those two things remaining I am posting this patch, so that > Jeevan Chalke can merge this patch into his patches and also merge > some of his changes related to mine and Robert's changes. Let me know > if this refactoring looks good. > Thanks Ashutosh for the refactoring patch. I will rebase my changes and will also resolve above two issues you have reported. > > -- > Best Wishes, > Ashutosh Bapat > EnterpriseDB Corporation > The Postgres Database Company > -- Jeevan Chalke Technical Architect, Product Development EnterpriseDB Corporation The Enterprise PostgreSQL Company
