Re: [HACKERS] Partition-wise aggregation/grouping

On Fri, Mar 16, 2018 at 3:19 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Mar 15, 2018 at 2:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> I wonder if we could simplify things by copying more information from
>> the parent grouping rel to the child grouping rels.
>
> On further review, it seems like a better idea is to generate the
> partial grouping relations from the grouping relations to which they
> correspond.  Attached is a series of proposed further refactoring
> patches.

Ok. That looks good.

>
> 0001 moves the creation of partially_grouped_rel downwards.  Instead
> of happening in create_grouping_paths(), it gets moved downward to
> add_paths_to_partial_grouping_rel(), which is renamed
> create_partial_grouping_paths() and now returns a pointer to new
> RelOptInfo.  This seems like a better design than what we've got now:
> it avoids creating the partially grouped relation if we don't need it,
> and it looks more like the other upper planner functions
> (create_grouping_paths, create_ordered_paths, etc.) which all create
> and return a new relation.

I liked that.

>
> 0002 moves the determination of which grouping strategies are possible
> upwards.  It represents them as a 'flags' variable with bits for
> GROUPING_CAN_USE_SORT, GROUPING_CAN_USE_HASH, and
> GROUPING_CAN_PARTIAL_AGG.  These are set in create_grouping_paths()
> and passed down to create_ordinary_grouping_paths().  The idea is that
> the flags value would be passed down to the partition-wise aggregate
> code which in turn would call create_ordinary_grouping_paths() for the
> child grouping relations, so that the relevant determinations are made
> only at the top level.

+1.

> This patch also renames can_parallel_agg to
> can_partial_agg and removes the parallelism-specific bits from it.

I think we need to update the comments in this function to use phrase
"partial aggregation" instead of "parallel aggregation". And I think
we need to change the conditions as well. For example if
parse->groupClause == NIL, why can't we do partial aggregation? This
is the classical case when we will need patial aggregation. Probably
we should test this with Jeevan's patches for partition-wise aggregate
to see if it considers partition-wise aggregate or not.

OR When parse->groupingSets is true, I can see why we can't use
parallel query, but we can still compute partial aggregates. This
condition doesn't hurt since partition-wise aggregation bails out when
there are grouping sets, so it's not that harmful here.

> To
> compensate for this, create_ordinary_grouping_paths() now tests the
> removed conditions instead.  This is all good stuff for partition-wise
> aggregate, since the grouped_rel->consider_parallel &&
> input_rel->partial_pathlist != NIL conditions can vary on a per-child
> basis but the rest of the stuff can't.  In some subsequent patch, the
> test should be pushed down inside create_partial_grouping_paths()
> itself, so that this function can handle both partial and non-partial
> paths as mentioned in the preceding paragraph.

I think can_parallel_agg() combines two conditions, whether partial
aggregation is possible and whether parallel aggregation is possible.
can_partial_agg() should have the first set and we should retain
can_parallel_agg() for the second set. We may then split
can_parallel_agg() into variant and invariant conditions i.e. the
conditions which change with input_rel and grouped_rel and those
don't.



>
> - create_partial_grouping_paths() is still doing
> get_agg_clause_costs() for the partial grouping target, which (I
> think) only needs to be done once.  Possibly we could handle that by
> having create_grouping_paths() do that work whenever it sets
> GROUPING_CAN_PARTIAL_AGG and pass the value downward.  You might
> complain that it won't get used unless either there are partial paths
> available for the input rel OR partition-wise aggregate is used --
> there's no point in partially aggregating a non-partial path at the
> top level.  We could just accept that as not a big deal, or maybe we
> can figure out how to make it conditional so that we only do it when
> either the input_rel has a partial path list or we have child rels.
> Or we could do as you did in your patches and save it when we compute
> it first, reusing it on each subsequent call.  Or maybe there's some
> other idea.

I am good with anything as long as we avoid repeated computation.

>
> I am sort of unclear whether we need/want GroupPathExtraData at all.
> What determines whether something gets passed via GroupPathExtraData
> or just as a separate argument?  If we have a rule that stuff that is
> common to all child grouped rels goes in there and other stuff
> doesn't, or stuff postgres_fdw needs goes in there and other stuff
> doesn't, then that might be OK.  But I'm not sure that there is such a
> rule in the v20 patches.

We have a single FDW hook for all the upper relations and that hook
can not accept grouping specific arguments. Either we need a separate
FDW hook for grouping OR we need some way of passing upper relation
specific information down to an FDW. I think some FDWs and extensions
will be happy if we provide them readymade decisions for can_sort,
can_hash, can_partial_agg etc. It will be good if they don't have to
translate the grouping target and havingQual for every child twice,
once for core and second time in the FDW. In all it looks like we need
some structure to hold that information so that we can pass it down
the hook. I am fine with two structures one variable and other
invariable. An upper operation can have one of them or both.

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