On Thu, Mar 8, 2018 at 8:02 PM, Jeevan Chalke <jeevan.cha...@enterprisedb.com> wrote: > > > On Thu, Mar 8, 2018 at 7:49 PM, Robert Haas <robertmh...@gmail.com> wrote: >> >> On Thu, Mar 8, 2018 at 9:15 AM, Jeevan Chalke >> <jeevan.cha...@enterprisedb.com> wrote: >> > I am not sure why we don't set reltarget into the grouped_rel too. >> > >> > But if we do so like we did in partially_grouped_rel, then it will be >> > lot >> > easier for partitionwise aggregate as then we don't have to pass target >> > to >> > functions creating paths like create_append_path. We now need to update >> > generate_gather_paths() to take target too as it is now being called on >> > grouped_rel in which reltarget is not set. >> > >> > But yes, if there is any specific reason we can't do so, then I think >> > the >> > same like Ashutosh Said. I didn't aware of such reason though. >> >> I see no problem with setting reltarget for the grouped_rel. Before >> we added partially_grouped_rel, that rel computed paths with two >> different targets: partial paths had the partial grouping target, and >> non-partial paths had the ordinary grouping target. But that's fixed >> now. > > > OK. > Will update my changes accordingly. > If we set reltarget into the grouped_rel now, then I don't need one of the > refactoring patch which is passing target to the path creation functions. >
For some reason we do not set reltarget of any of the upper relations. I don't know why, neither browsing through the comments in grouping_planner(), including the one below before the code that creates an array of upper relation targets. /* * Save the various upper-rel PathTargets we just computed into * root->upper_targets[]. The core code doesn't use this, but it * provides a convenient place for extensions to get at the info. For * consistency, we save all the intermediate targets, even though some * of the corresponding upperrels might not be needed for this query. */ Why don't we just set those in the corresponding RelOptInfos? May be we should do that for all the upper rels and not just grouping_rel. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company