On Wed, Mar 7, 2018 at 8:07 PM, Jeevan Chalke <jeevan.cha...@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? + +/* + * 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