On Fri, Feb 2, 2018 at 8:25 AM, Jeevan Chalke <jeevan.cha...@enterprisedb.com> wrote: >> 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
So in the new scheme I'm proposing, you've got a partially_grouped_rel and a grouped_rel. So all paths of type #1 go into partially_grouped_rel->partial_pathlist, paths of type #2 go into partially_grouped_rel->pathlist, type #3 (if we have any) goes into grouped_rel->partial_pathlist, and type #4 goes into grouped_rel->pathlist. > 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. Oh, I didn't realize that wasn't working already. I agree that it's a separate task from this patch, but it's really too bad that it doesn't already work. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
