On Thu, Mar 15, 2018 at 3:38 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote:
> > The patchset needs rebase. I have rebased those on the latest head > and made following changes. > > Argument force_partial_agg is added after output arguments to > make_grouping_rels(). Moved it before output arguemnts to keep input and > output > arguments separate. > > Also moved the comment about creating partial aggregate paths before full > aggregate paths in make_grouping_rels() moved to > populate_grouping_rels_with_paths(). > > In create_grouping_paths() moved call to try_degenerate_grouping_paths() > before > computing extra grouping information. > Thanks for these changes. > > Some more comment changes in the attached patch set. > > + * > + * With partitionwise aggregates, we may have childrel's pathlist > empty > + * if we are doing partial aggregation. Thus do this only if > childrel's > + * pathlist is not NIL. > */ > - if (childrel->cheapest_total_path->param_info == NULL) > + if (childrel->pathlist != NIL && > + childrel->cheapest_total_path->param_info == NULL) > accumulate_append_subpath(childrel->cheapest_total_path, > &subpaths, NULL); > I thought we got rid of this code. Why has it come back? May be the comment > should point to a function where this case happen. > Oops. My mistake. I have added it back while working on your comments. And at that time we were creating partially_grouped_rel unconditionally. I was getting segfault here. But now, we create partially_grouped_rel only when needed, thanks for your refactoring work. Thus no need of this guard. Removed in attached patch set. > In populate_grouping_rels_with_paths(), we need to pass extra to > extension hook create_upper_paths_hook similar to what > add_paths_to_joinrel() does. > Hmm.. you are right. Done. > > Also, we aren't passing is_partial_agg to FDW hook, so it won't know > whether to compute partial or full aggregates. I think the check > 5296 /* Partial aggregates are not supported. */ > 5297 if (extra->partial_partitionwise_grouping) > 5298 return; > in add_foreign_grouping_paths() is wrong. It's checking whether the > children of the given relation will produce partial aggregates or not. > But it is supposed to check whether the given relation should produce > partial aggregates or not. I think we need to include is_partial_agg > in GroupPathExtraData so that it gets passed to FDWs. If we do so, we > need to add a comment in the prologue of GroupPathExtraData to > disambiguate usage of is_partial_agg and > partial_partitionwise_grouping. > Sorry, I mis-interpreted this. Reworked and added is_partial_aggregation in GroupPathExtraData. > In current create_grouping_paths() (without any of your patches > applied) we first create partial paths in partially grouped rel and > then add parallel path to grouped rel using those partial paths. Then > we hand over this to FDW and extension hooks, which may add partial > paths, which might throw away a partial path used to create a parallel > path in grouped rel causing a segfault. I think this bug exists since > we introduced parallel aggregation or upper relation refactoring > whichever happened later. Introduction of partially grouped rel has > just made it visible. > Yes. That's why I needed to create partitionwise aggregation paths before we create any normal paths. Yeah, but if this issue needs to be taken care, it should be a separate patch. However, as noticed by Ashutosh Bapat, after refactoring, we no more try to create partitionwise paths, rather if possible we do create them. So inlined with create_grouping_paths() I have renamed it to create_partitionwise_grouping_paths() in attached patch-set. Thanks > -- > Best Wishes, > Ashutosh Bapat > EnterpriseDB Corporation > The Postgres Database Company > -- Jeevan Chalke Technical Architect, Product Development EnterpriseDB Corporation The Enterprise PostgreSQL Company
partition-wise-agg-v20.tar.gz
Description: application/gzip
