Re: BUG #15613: Bug in PG Planner for Foreign Data Wrappers
От | Etsuro Fujita |
---|---|
Тема | Re: BUG #15613: Bug in PG Planner for Foreign Data Wrappers |
Дата | |
Msg-id | 5C52C769.5090600@lab.ntt.co.jp обсуждение исходный текст |
Ответ на | Re: BUG #15613: Bug in PG Planner for Foreign Data Wrappers (Tom Lane <tgl@sss.pgh.pa.us>) |
Ответы |
Re: BUG #15613: Bug in PG Planner for Foreign Data Wrappers
(Tom Lane <tgl@sss.pgh.pa.us>)
|
Список | pgsql-bugs |
(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
В списке pgsql-bugs по дате отправления:
Предыдущее
От: PG Bug reporting formДата:
Сообщение: BUG #15614: Query plan: buffer stats from workers in child operations discarded.
Следующее
От: Adrien NAYRATДата:
Сообщение: Re: BUG #15614: Query plan: buffer stats from workers in childoperations discarded.