Re: Problems with plan estimates in postgres_fdw

(2019/02/08 21:35), Etsuro Fujita wrote:
> (2019/02/08 2:04), Antonin Houska wrote:
>> Some comments on coding:
>>
>> 0001-postgres_fdw-Perform-UPPERREL_ORDERED-step-remotely-v3.patch
>> -----------------------------------------------------------------
>>
>> * I'm not sure if UPPERREL_ORDERD should to deal with LIMIT at all.
>> Note that
>> grouping_planner() does not call create_limit_path() until it's done with
>> create_ordered_paths(), and create_ordered_paths() is where the FDW is
>> requested to add its paths to UPPERREL_ORDERED relation.
>
> Maybe I'm missing something, but the 0001 patch doesn't deal with LIMIT
> at all. I added the parameter limit_tuples to PgFdwPathExtraData so that
> we can pass limit_tuples=-1, which means no LIMIT, to cost_sort(), though.

As proposed by you downthread, I removed the limit_tuples variable at 
all from that patch.

>> * add_foreign_ordered_paths()
>>
>> Exprssion root->upper_targets[UPPERREL_FINAL] is used to access the
>> target.

I modified that patch so that we use 
root->upper_targets[UPPERREL_ORDERED], not 
root->upper_targets[UPPERREL_FINAL].

>> * regression tests: I think test(s) should be added for queries that have
>> ORDER BY clause but do not have GROUP BY (and also no LIMIT / OFFSET)
>> clause. I haven't noticed such tests.
>
> Will do.

I noticed that such queries would be processed by what we already have 
for sort pushdown (ie, add_paths_with_pathkeys_for_rel()).  I might be 
missing something, though.

>> 0002-postgres_fdw-Perform-UPPERREL_FINAL-step-remotely-v3.patch
>> ---------------------------------------------------------------

>> * adjust_limit_rows_costs()
>>
>> Doesn't this function address more generic problem than the use of
>> postgres_fdw? If so, then it should be submitted as a separate patch.
>> BTW, the
>> function is only used in pathnode.c, so it should be declared static.
>
> Actually, this is simple refactoring for create_limit_path() so that
> that function can be shared with postgres_fdw. See
> estimate_path_cost_size(). I'll separate that refactoring in the next
> version of the patch set.

Done.

Other changes:

* In add_foreign_ordered_paths() and add_foreign_final_paths(), I 
replaced create_foreignscan_path() with the new function 
create_foreign_upper_path() added by the commit [1].

* In 0003-postgres_fdw-Perform-UPPERREL_FINAL-step-remotely.patch, I 
modified estimate_path_cost_size() so that the costs are adjusted to 
ensure we'll prefer performing LIMIT remotely, after factoring in some 
additional cost to account for connection overhead, not before, because 
that would make the adjustment more robust against changes to such cost.

* Revised comments a bit.

* Rebased the patchset to the latest HEAD.

Attached is an updated version of the patchset.  I plan to add more 
comments in the next version.

Best regards,
Etsuro Fujita

[1] 
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=34ea1ab7fd305afe1124a6e73ada0ebae04b6ebb