Re: [HACKERS] Partition-wise aggregation/grouping

On Fri, Feb 2, 2018 at 1:41 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

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

That was just a quick patch to make sure is this what you meant.
Yes, it need some more work as you suggested and 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.
>

OK. Got it.

Will try to find a common place for them and will also check how it goes
with your suggested design change.

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

The new paths are specific to partition-wise aggregates and I thought
better to keep them separately without interfering with grouped_rel
pathlist/partial_pathlist. And as you said, I didn't find a better place
that its own structure.

> >> 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
>
>
add_paths_to_append_rel() -> generate_mergeappend_paths() does not consider
partial_pathlist. Thus we will never see MergeAppend over parallel scan
given by partial_pathlist. And thus plan like:
-> Gather Merge
-> MergeAppend
is not possible with current HEAD.

Are you suggesting we should implement that here? I think that itself is a
separate task.

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

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