Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Feb 1, 2018 at 8:59 AM, Jeevan Chalke
<jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
> I wrote a patch for this (on current HEAD) and attached separately here.
> Please have a look.

Yes, this is approximately what I had in mind, though it needs more
work (e.g. it doesn't removing the clearing of the grouped_rel's
partial_pathlist, which should no longer be necessary; also, it needs
substantial comment updates).

>> 1. Parallel partition-wise aggregate, grouping key doesn't contain
>> partition key. This should just be a matter of adding additional
>> Append paths to partially_grouped_rel->partial_pathlist. The existing
>> code already knows how to stick a Gather and FinalizeAggregate step on
>> top of that, and I don't see why that logic would need any
>> modification or addition. An Append of child partial-grouping paths
>> should be producing the same output as a partial grouping of an
>> Append, except that the former case might produce more separate groups
>> that need to be merged; but that should be OK: we can just throw all
>> the paths into the same path list and let the cheapest one win.
>
> For any partial aggregation we need to add finalization step after we are
> done with the APPEND i.e. post add_paths_to_append_rel(). Given that we need
> to replicate the logic of sticking Gather and FinalizeAggregate step at
> later stage. This is what exactly done in create_partition_agg_paths().
> Am I missing something here?

The problem is that create_partition_agg_paths() is doing *exactly*
same thing that add_paths_to_grouping_rel() is already doing inside
the blocks that say if (grouped_rel->partial_pathlist). We don't need
two copies of that code. Both of those places except to take a
partial path that has been partially aggregated and produce a
non-partial path that is fully aggregated. We do not need or want two
copies of that code.

Here's another way to look at it. We have four kinds of things.

1. Partially aggregated partial paths
2. Partially aggregated non-partial paths
3. Fully aggregated partial paths
4. Fully aggregated non-partial paths

The current code only ever generates paths of type #1 and #4; this
patch will add paths of type #2 as well, and maybe also type #3. But
the way you've got it, the existing paths of type #1 go into the
grouping_rel's partial_pathlist, and the new paths of type #1 go into
the OtherUpperPathExtraData's partial_paths list. Maybe there's a
good reason why we should keep them separate, but I'm inclined to
think they should all be going into the same list.

>> Overall, what I'm trying to argue for here is making this feature look
>> less like its own separate thing and more like part of the general
>> mechanism we've already got: partial paths would turn into regular
>> paths via generate_gather_paths(), and partially aggregated paths
>> would turn into fully aggregated paths by adding FinalizeAggregate.
>> The existing special case that allows us to build a non-partial, fully
>> aggregated path from a partial, partially-aggregated path would be
>> preserved.
>>
>> I think this would probably eliminate some other problems I see in the
>> existing design as well. For example, create_partition_agg_paths
>> doesn't consider using Gather Merge, but that might be a win.
>
> Append path is always non-sorted and has no pathkeys. Thus Gather Merge over
> an Append path seems infeasible, isn't it?

We currently never generate an Append path with pathkeys, but we do
generate MergeAppend paths with pathkeys, as in the following example:

rhaas=# create table foo (a int, b text) partition by range (a);
CREATE TABLE
rhaas=# create index on foo (a);
CREATE INDEX
rhaas=# create table foo1 partition of foo for values from (0) to (1000000);
CREATE TABLE
rhaas=# create table foo2 partition of foo for values from (1000000)
to (2000000);
CREATE TABLE
rhaas=# select * from foo foo order by a;
a | b
---+---
(0 rows)
rhaas=# explain select * from foo foo order by a;
QUERY PLAN
----------------------------------------------------------------------------------------
Merge Append (cost=0.32..145.47 rows=2540 width=36)
Sort Key: foo.a
-> Index Scan using foo1_a_idx on foo1 foo (cost=0.15..63.20
rows=1270 width=36)
-> Index Scan using foo2_a_idx on foo2 foo_1 (cost=0.15..63.20
rows=1270 width=36)
(4 rows)

Actually, in this example, the MergeAppend could be safely converted
into an Append, because the partitions are in bound order, and
somebody already proposed a patch for that.

The point is that we want to be able to get plans like this:

Finalize GroupAggregate
-> Gather Merge
-> MergeAppend
-> Partial GroupAggregate
-> Parallel Index Scan on t1
-> Partial GroupAggregate
-> Parallel Index Scan on t2
-> Partial GroupAggregate
-> Parallel Index Scan on t3

If we only consider Gather, not Gather Merge, when turning a partially
aggregated partial path into a non-partial path, then we end up having
to insert a Sort node if we want to perform a Finalize GroupAggregate
step.

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