From: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: srinivasan.sa@zohocorp.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15613: Bug in PG Planner for Foreign Data Wrappers
Date: Thu, 31 Jan 2019 19:01:13 +0900
Message-ID: <5C52C769.5090600@lab.ntt.co.jp> (raw)
In-Reply-To: <24678.1548870484@sss.pgh.pa.us>
References: <15613-092be1be9576c728@postgresql.org>
<5C51993C.2070606@lab.ntt.co.jp>
<19832.1548865557@sss.pgh.pa.us>
<24678.1548870484@sss.pgh.pa.us>
(2019/01/31 2:48), Tom Lane wrote:
> 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?
> 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.)
#2 seems like a good idea, as it would make FDW authors' life easy.
> 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.
Agreed.
Thank you for working on this issue, as usual!
Best regards,
Etsuro Fujita
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: fujita.etsuro@lab.ntt.co.jp, tgl@sss.pgh.pa.us, srinivasan.sa@zohocorp.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15613: Bug in PG Planner for Foreign Data Wrappers
In-Reply-To: <5C52C769.5090600@lab.ntt.co.jp>
* 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