|From:||Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>|
|To:||Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>|
|Subject:||Re: BUG #15613: Bug in PG Planner for Foreign Data Wrappers|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
(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:
> NIL, /* no pathkeys */
> - NULL, /* no outer rel either */
> + baserel->lateral_relids,
> NULL, /* no extra plan */
> 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.
Thank you for working on this issue, as usual!
|Next Message||Adrien NAYRAT||2019-01-31 10:28:58||Re: BUG #15614: Query plan: buffer stats from workers in child operations discarded.|
|Previous Message||PG Bug reporting form||2019-01-31 09:33:57||BUG #15614: Query plan: buffer stats from workers in child operations discarded.|