Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 15, 2018 at 3:38 PM, Ashutosh Bapat <
ashutosh(dot)bapat(at)enterprisedb(dot)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