From: Tom Lane <tgl@sss.pgh.pa.us>
To: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
Cc: srinivasan.sa@zohocorp.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15613: Bug in PG Planner for Foreign Data Wrappers
Date: Wed, 30 Jan 2019 12:48:04 -0500
Message-ID: <24678.1548870484@sss.pgh.pa.us> (raw)
In-Reply-To: <19832.1548865557@sss.pgh.pa.us>
References: <15613-092be1be9576c728@postgresql.org>
<5C51993C.2070606@lab.ntt.co.jp>
<19832.1548865557@sss.pgh.pa.us>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, fujita.etsuro@lab.ntt.co.jp, srinivasan.sa@zohocorp.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15613: Bug in PG Planner for Foreign Data Wrappers
In-Reply-To: <24678.1548870484@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox