Message-ID: <5C5A5F75.5040203@lab.ntt.co.jp>
Date: Wed, 06 Feb 2019 13:15:49 +0900
From: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64;
 rv:11.0) Gecko/20120327 Thunderbird/11.0.1
MIME-Version: 1.0
Subject: Re: BUG #15613: Bug in PG Planner for Foreign Data Wrappers
References: <15613-092be1be9576c728@postgresql.org>
 <5C51993C.2070606@lab.ntt.co.jp> <19832.1548865557@sss.pgh.pa.us>
 <24678.1548870484@sss.pgh.pa.us> <5C52C769.5090600@lab.ntt.co.jp>
 <19209.1548957996@sss.pgh.pa.us>
In-Reply-To: <19209.1548957996@sss.pgh.pa.us>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: srinivasan.sa@zohocorp.com, pgsql-bugs@lists.postgresql.org
Precedence: bulk

(2019/02/01 3:06), Tom Lane wrote:
> Etsuro Fujita<fujita.etsuro@lab.ntt.co.jp>  writes:
>> (2019/01/31 2:48), Tom Lane wrote:
>>> So I see two alternatives for fixing this aspect of the problem:
>>>
>>> 1. Just change file_fdw and postgres_fdw as above, and hope that
>>> authors of extension FDWs get the word.
>>>
>>> 2. Modify create_foreignscan_path so that it doesn't simply trust
>>> required_outer to be correct, but forcibly adds rel->lateral_relids
>>> into it.  This would fix the problem without requiring FDWs to be
>>> on board, but it seems kinda grotty, and it penalizes FDWs that
>>> have gone to the trouble of computing the right required_outer relids
>>> in the first place.  (It's somewhat amusing that postgres_fdw
>>> appears to get this right when it generates parameterized paths,
>>> but not in the base case.)
>
>> #2 seems like a good idea, as it would make FDW authors' life easy.
>
> I started to fix this, and soon noticed what seems a worse problem:
> postgres_fdw is using create_foreignscan_path to construct Paths for
> join relations and upperrels.  This is utterly broken.  That function
> was only designed to produce paths for baserels, which is why it uses
> get_baserel_parampathinfo.  At the very least we're getting wrong
> rowcount estimates for parameterized joinrels (compare
> get_joinrel_parampathinfo), and it seems possible that we're actually
> getting wrong plans with the wrong set of movable join clauses being
> applied.  And I have no idea what might go wrong for upperrels, though
> I think those are never parameterized so it might accidentally not fail.

Ah, you are right.  I also noticed that when I proposed parameterized 
foreign joins for postgres_fdw two years ago, but I forgot that.

> We could either split the function into two or three functions, or add
> still more overhead to it to notice what kind of relation has been
> passed and adjust its behavior for that.  I'm not really thrilled with
> the latter: the fact that it's called create_foreignSCAN_path means,
> to me, that it's not supposed to be used for anything but baserel
> cases.

I don't have any strong opinion on that.

> I think one big question here is how many external FDWs may have
> copied postgres_fdw's remote-join handling.  If we just have to
> fix postgres_fdw, my thoughts about what to do are probably
> different than if we have to try to avoid making third-party callers
> more broken than they are already.

As far as I know oracle_fdw supports join pushdown the same way as 
postgres_fdw [1], but other than that, I guess there are few if any.

Best regards,
Etsuro Fujita

[1] http://laurenz.github.io/oracle_fdw/