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

I wrote:
> So there are a few interesting questions here:
> * Why was the IS NOT NULL pushed down to the wrong place?
> * Having made that mistake, why didn't the NLP mechanism fix it
> and create a valid plan anyway?

Hmm ... it looks like there is indeed more than one bug here.

The source of all the problems seems to be that the IS NOT NULL
variable is getting wrapped into a PlaceHolderVar incorrectly(?):
by the time it gets to distribute_qual_to_rels, it looks like

{NULLTEST
:arg
{PLACEHOLDERVAR
:phexpr
{VAR
:varno 1
:varattno 2
:vartype 20
:vartypmod -1
:varcollid 0
:varlevelsup 0
:varnoold 1
:varoattno 2
:location 275
}
:phrels (b 6)
:phid 1
:phlevelsup 0
}
:nulltesttype 1
:argisrow false
:location 456
}

I haven't tracked down why that's happening, but this parsetree
clearly says that Var 1/2 (that is, ref_0.t1_col2) should get
evaluated at the scan of RTE 6 (ref_1), which would then make
ref_1 be the natural place to do the filter check too. So given
that we built this PHV, the plan should come out the way it did.
The reason that we fail to make an NLP to make it work is that
create_foreignscan_plan doesn't think it needs to perform
replace_nestloop_params, because the Path is marked with null
param_info. And that is because fileGetForeignPaths unconditionally
passes required_outer = NULL to create_foreignscan_path.

What *ought* to be happening, of course, is what's documented in
most of the core path-creation functions, such as set_plain_rel_pathlist:

/*
* We don't support pushing join clauses into the quals of a seqscan, but
* it could still have required parameterization due to LATERAL refs in
* its tlist.
*/
required_outer = rel->lateral_relids;

and indeed changing fileGetForeignPaths like this:

startup_cost,
total_cost,
NIL, /* no pathkeys */
- NULL, /* no outer rel either */
+ baserel->lateral_relids,
NULL, /* no extra plan */
coptions));

is enough to make the failure go away: we still see the IS NOT NULL
getting applied in the arguably-wrong place, but the reference is
correctly parameterized so that the plan executes without error.

Observe that this *is* a bug independently of whether the PlaceHolderVar
generation is correct or not. Now that we've recognized that these calls
are wrong, it should be possible to create test cases that fail for these
FDWs without relying on that.

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

A variant of #2 that might make it seem less grotty is to decide that
*all* the path-node-creation functions in pathnode.c should be
responsible for adding rel->lateral_relids into the path's outerrels,
instead of expecting callers to do so, which would allow for some
simplification in (at least some of) the callers. This would be
a bit invasive, so I'd be inclined not to do it like that in the
back branches, but perhaps it's sensible in HEAD.

I don't have a huge amount of faith in #1, so I think we ought to do
one or the other variant of #2.

Thoughts?

regards, tom lane