Message-ID: <5C52C769.5090600@lab.ntt.co.jp>
Date: Thu, 31 Jan 2019 19:01:13 +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>
In-Reply-To: <24678.1548870484@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/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