(2019/02/12 20:43), Antonin Houska wrote:
> Etsuro Fujita<fujita.etsuro@lab.ntt.co.jp> wrote:
>> Here are my review comments:
>>
>> root->upper_targets[UPPERREL_FINAL] = final_target;
>> + root->upper_targets[UPPERREL_ORDERED] = final_target;
>> root->upper_targets[UPPERREL_WINDOW] = sort_input_target;
>> root->upper_targets[UPPERREL_GROUP_AGG] = grouping_target;
>>
>> I think that's a good idea. I think we'll soon need the PathTarget for
>> UPPERREL_DISTINCT, so how about saving that as well like this?
>>
>> root->upper_targets[UPPERREL_FINAL] = final_target;
>> + root->upper_targets[UPPERREL_ORDERED] = final_target;
>> + root->upper_targets[UPPERREL_DISTINCT] = sort_input_target;
>> root->upper_targets[UPPERREL_WINDOW] = sort_input_target;
>> root->upper_targets[UPPERREL_GROUP_AGG] = grouping_target;
>
> I see nothing wrong about this.
Cool!
>> Another is about this:
>>
>> /*
>> + * Set reltarget so that it's consistent with the paths. Also it's more
>> + * convenient for FDW to find the target here.
>> + */
>> + ordered_rel->reltarget = target;
>>
>> I think this is correct if there are no SRFs in the targetlist, but otherwise
>> I'm not sure this is really correct because the relation's reltarget would not
>> match the target of paths added to the relation after adjust_paths_for_srfs().
>
> I wouldn't expect problem here. create_grouping_paths() also sets the
> reltarget although adjust_paths_for_srfs() can be alled on grouped_rel
> afterwards.
Yeah, but as I said in a previous email, I think the root->upper_targets
change would be enough at least currently for FDWs to consider
performing post-UPPERREL_GROUP_AGG steps remotely, as shown in my
patches, so I'm inclined to leave the retarget change for future work.
Best regards,
Etsuro Fujita