Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 15, 2018 at 3:38 PM, Ashutosh Bapat <
ashutosh.bapat@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