Re: Problems with plan estimates in postgres_fdw

(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