Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 8, 2018 at 8:02 PM, Jeevan Chalke
<jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
>
>
> On Thu, Mar 8, 2018 at 7:49 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>
>> On Thu, Mar 8, 2018 at 9:15 AM, Jeevan Chalke
>> <jeevan(dot)chalke(at)enterprisedb(dot)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