On Fri, Mar 9, 2018 at 4:21 PM, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: > On Thu, Mar 8, 2018 at 7:31 PM, Robert Haas <robertmhaas@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