On Fri, Feb 2, 2018 at 1:41 AM, Robert Haas <robertmh...@gmail.com> wrote:
> On Thu, Feb 1, 2018 at 8:59 AM, Jeevan Chalke > <jeevan.cha...@enterprisedb.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
