Re: postgres_fdw: another oddity in costing aggregate pushdown paths

(2019/02/22 23:10), Antonin Houska wrote:
> Etsuro Fujita<fujita.etsuro@lab.ntt.co.jp>  wrote:
>> As mentioned in the near thread, I think there is another oversight in
>> the cost estimation for aggregate pushdown paths in postgres_fdw, IIUC.
>>   When costing an aggregate pushdown path using local statistics, we
>> re-use the estimated costs of implementing the underlying scan/join
>> relation, cached in the relation's PgFdwRelationInfo (ie,
>> rel_startup_cost and rel_total_cost).  Since these costs wouldn't yet
>> contain the costs of evaluating the final scan/join target, as tlist
>> replacement by apply_scanjoin_target_to_paths() is performed afterwards.
>>   So I think we need to adjust these costs so that the tlist eval costs
>> are included, but ISTM that estimate_path_cost_size() forgot to do so.
>> Attached is a patch for fixing this issue.
>
> I think the following comment in apply_scanjoin_target_to_paths() should
> mention that FDWs rely on the new value of reltarget.
>
> 	/*
> 	 * Update the reltarget.  This may not be strictly necessary in all cases,
> 	 * but it is at least necessary when create_append_path() gets called
> 	 * below directly or indirectly, since that function uses the reltarget as
> 	 * the pathtarget for the resulting path.  It seems like a good idea to do
> 	 * it unconditionally.
> 	 */
> 	rel->reltarget = llast_node(PathTarget, scanjoin_targets);

Agreed.  How about mentioning that like the attached?  In addition, I 
added another assertion to estimate_path_cost_size() in that patch.

Thanks for the review!

Best regards,
Etsuro Fujita