Re: [HACKERS] Partition-wise aggregation/grouping

On Wed, Mar 14, 2018 at 7:51 PM, Jeevan Chalke
<jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
>
>
> On Tue, Mar 13, 2018 at 7:43 PM, Jeevan Chalke
> <jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
>>
>> Hi,
>>
>> The patch-set is complete now. But still, there is a scope of some comment
>> improvements due to all these refactorings. I will work on it. Also, need to
>> update few documentations and indentations etc. Will post those changes in
>> next patch-set. But meanwhile, this patch-set is ready to review.
>
>
> Attached complete patch-set.
>
> Fixed all remaining documentation, indentation and comments.
> Also incorporated few comments improvements provided off-list by 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 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.

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.

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.

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.

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.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company