Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 8, 2018 at 1:15 PM, Ashutosh Bapat <
ashutosh.bapat@enterprisedb.com> wrote:

> On Wed, Mar 7, 2018 at 8:07 PM, Jeevan Chalke
> <jeevan.chalke@enterprisedb.com> wrote:
> Here are some more review comments esp. on
> try_partitionwise_grouping() function. BTW name of that function
> doesn't go in sync with enable_partitionwise_aggregation (which btw is
> in sync with enable_fooagg GUCs). But it goes in sync with
> create_grouping_paths(). It looks like we have already confused
> aggregation and grouping e.g. enable_hashagg may affect path creation
> when there is no aggregation involved i.e. only grouping is involved
> but create_grouping_paths will create paths when there is no grouping
> involved. Generally it looks like the function names use grouping to
> mean both aggregation and grouping but GUCs use aggregation to mean
> both of those. So, the naming in this patch looks consistent with the
> current naming conventions.
>
> +        grouped_rel->part_scheme = input_rel->part_scheme;
> +        grouped_rel->nparts = nparts;
> +        grouped_rel->boundinfo = input_rel->boundinfo;
> +        grouped_rel->part_rels = part_rels;
>
> You need to set the part_exprs which will provide partition keys for this
> partitioned relation. I think, we should include all the part_exprs of
> input_rel which are part of GROUP BY clause. Since any other expressions in
> part_exprs are not part of GROUP BY clause, they can not appear in the
> targetlist without an aggregate on top. So they can't be part of the
> partition
> keys of the grouped relation.
>
> In create_grouping_paths() we fetch both partial as well as fully grouped
> rel
> for given input relation. But in case of partial aggregation, we don't need
> fully grouped rel since we are not computing full aggregates for the
> children.
> Since fetch_upper_rel() creates a relation when one doesn't exist, we are
> unnecessarily creating fully grouped rels in this case. For thousands of
> partitions that's a lot of memory wasted.
>
> I see a similar issue with create_grouping_paths() when we are computing
> only
> full aggregates (either because partial aggregation is not possible or
> because
> parallelism is not possible). In that case, we unconditionally create
> partially
> grouped rels. That too would waste a lot of memory.
>
> I think that partially_grouped_rel, when required, is partitioned
> irrespective
> of whether we do full aggregation per partition or not. So, if we have its
> part_rels and other partitioning properties set. I agree that right now we
> won't use this information anywhere. It may be useful, in case we device a
> way
> to use partially_grouped_rel directly without using grouped_rel for
> planning
> beyond grouping, which seems unlikely.
>
> +
> +        /*
> +         * Parallel aggregation requires partial target, so compute it
> here
> +         * and translate all vars. For partial aggregation, we need it
> +         * anyways.
> +         */
> +        partial_target = make_partial_grouping_target(root, target,
> +                                                      havingQual);
>
> Don't we have this available in partially_grouped_rel?
>
> That shows one asymmetry that Robert's refactoring has introduced. We
> don't set
> reltarget of grouped_rel but set reltarget of partially_grouped_rel. If
> reltarget of grouped_rel changes across paths (the reason why we have not
> set
> it in grouped_rel), shouldn't reltarget of partially grouped paths change
> accordingly?
>

I am not sure why we don't set reltarget into the grouped_rel too.

But if we do so like we did in partially_grouped_rel, then it will be lot
easier for partitionwise aggregate as then we don't have to pass target to
functions creating paths like create_append_path. We now need to update
generate_gather_paths() to take target too as it is now being called on
grouped_rel in which reltarget is not set.

But yes, if there is any specific reason we can't do so, then I think the
same like Ashutosh Said. I didn't aware of such reason though.


> +
> +/*
> + * group_by_has_partkey
> + *
> + * Returns true, if all the partition keys of the given relation are part
> of
> + * the GROUP BY clauses, false otherwise.
> + */
> +static bool
> +group_by_has_partkey(RelOptInfo *input_rel, PathTarget *target,
> +                     List *groupClause)
>
> We could modify this function to return the list of part_exprs which are
> part
> of group clause and use that as the partition keys of the grouped_rel if
> required. If group by doesn't have all the partition keys, the function
> would
> return a NULL list.
>
> Right now, in case of full aggregation, partially_grouped_rel is populated
> with
> the partial paths created by adding partial aggregation to the partial
> paths of
> input relation. But we are not trying to create partial paths by (parallel)
> appending the (non)partial paths from the child partially_grouped_rel.
> Have we
> thought about that? Would such paths have different shapes from the ones
> that
> we create now and will they be better?
>
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company
>



-- 
Jeevan Chalke
Technical Architect, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company