Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 15, 2018 at 2:46 PM, Robert Haas <robertmhaas(at)gmail(dot)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.

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. One possible objection to this line of
attack is that Jeevan's
0006-Implement-partitionwise-aggregation-paths-for-partit.patch patch
adds an additional Boolean argument to that function so that it can be
called twice, once for partial paths and a second time for non-partial
paths. But it looks to me like we should instead just add separate
handling to this function for the pathlist in each place where we're
currently handling the partial_pathlist. That's more like what we do
elsewhere and avoids complicating the code with a bunch of
conditionals. To make the generation of partially_grouped_rel from
grouped_rel work cleanly, this also sets grouped_rel's reltarget.

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. This patch also renames can_parallel_agg to
can_partial_agg and removes the parallelism-specific bits from it. 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.

0003 is a cleanup patch. It removes the grouping target as a separate
argument from a bunch of places that no longer need it given that 0001
sets grouped_rel->reltarget.

I think 0001 and 0002 together get us pretty close to having the stuff
that should be done for every child rel
(create_ordinary_grouping_paths) disentangled from the stuff that
should be done only once (create_grouping_paths). There are a couple
of exceptions:

- 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.

- These patches don't do anything about
create_partial_grouping_paths() and create_ordinary_grouping_paths()
directly referencing parse->targetList and parse->havingQual. I think
that we could add those as additional arguments to
create_ordinary_grouping_paths(). create_grouping_paths() would pass
the values taken from "parse" and partition-wise join would pass down
translated versions.

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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company