Re: Problems with plan estimates in postgres_fdw

From: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
To: Antonin Houska <ah(at)cybertec(dot)at>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Problems with plan estimates in postgres_fdw
Date: 2019-02-20 12:25:51
Message-ID: 5C6D474F.5010703@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

(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

Attachment Content-Type Size
0001-postgres_fdw-Perform-UPPERREL_ORDERED-step-remotely-v4.patch text/x-patch 44.1 KB
0002-Refactor-create_limit_path-to-share-cost-adjustment-v4.patch text/x-patch 4.8 KB
0003-postgres_fdw-Perform-UPPERREL_FINAL-step-remotely-v4.patch text/x-patch 99.0 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Etsuro Fujita 2019-02-20 12:30:20 Re: 2019-03 CF Summary / Review - Tranche #2
Previous Message Michael Meskes 2019-02-20 12:17:12 Re: SQL statement PREPARE does not work in ECPG