|From:||Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)com>|
|To:||Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>|
|Cc:||Robert Haas <robertmhaas(at)gmail(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>, Rajkumar Raghuwanshi <rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: [HACKERS] Partition-wise aggregation/grouping|
|Views:||Raw Message | Whole Thread | Download mbox|
On Thu, Mar 15, 2018 at 3:38 PM, Ashutosh Bapat <
> 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
> arguments separate.
> Also moved the comment about creating partial aggregate paths before full
> aggregate paths in make_grouping_rels() moved to
> In create_grouping_paths() moved call to try_degenerate_grouping_paths()
> 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
> + * if we are doing partial aggregation. Thus do this only if
> + * pathlist is not NIL.
> - if (childrel->cheapest_total_path->param_info == NULL)
> + if (childrel->pathlist != NIL &&
> + childrel->cheapest_total_path->param_info == NULL)
> &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
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
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.
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company
Technical Architect, Product Development
The Enterprise PostgreSQL Company
|Next Message||Tom Lane||2018-03-15 13:58:43||Re: INOUT parameters in procedures|
|Previous Message||Robert Haas||2018-03-15 13:46:26||Re: [HACKERS] Removing [Merge]Append nodes which contain a single subpath|