(2019/02/14 18:44), Etsuro Fujita wrote:
> (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.
So, here is an updated patch. If there are no objections from you or
anyone else, I'll commit the patch as a preliminary one for what's
proposed in this thread.
Best regards,
Etsuro Fujita