Re: BUG #15613: Bug in PG Planner for Foreign Data Wrappers

Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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.

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 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.

regards, tom lane