On Wed, May 29, 2019 at 6:12 AM Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: > > On 2019/05/28 20:26, Amit Kapila wrote: > > On Tue, May 28, 2019 at 12:29 PM Amit Langote wrote: > >> Seeing that the crash occurs due to postgres_fdw relying on > >> es_result_relation_info being set when initializing a "direct > >> modification" plan on foreign tables managed by it, we could change the > >> comment to say that instead. Note that allowing "direct modification" of > >> foreign tables is a core feature, so there's no postgres_fdw-specific > >> behavior here; there may be other FDWs that support "direct modification" > >> plans and so likewise rely on es_result_relation_info being set. > > > > > > Can we ensure some way that only FDW's rely on it not any other part > > of the code? > > Hmm, I can't think of any way of doing than other than manual inspection. > We are sure that no piece of core code relies on it in the ExecInitNode() > code path. Apparently FDWs may, as we've found out here. Now that I've > looked around, maybe other loadable modules may too, by way of (only?) > Custom nodes. I don't see any other way to hook into ExecInitNode(), so > maybe that's it. > > So, maybe reword a bit as: > > diff --git a/src/backend/executor/nodeModifyTable.c > b/src/backend/executor/nodeModifyTable.c > index a3c0e91543..95545c9472 100644 > --- a/src/backend/executor/nodeModifyTable.c > +++ b/src/backend/executor/nodeModifyTable.c > @@ -2316,7 +2316,7 @@ ExecInitModifyTable(ModifyTable *node, EState > *estate, int eflags) > * verify that the proposed target relations are valid and open their > * indexes for insertion of new index entries. Note we *must* set > * estate->es_result_relation_info correctly while we initialize each > - * sub-plan; ExecContextForcesOids depends on that! > + * sub-plan; external modules such as FDWs may depend on that. >
I think it will be better to include postgres_fdw in the comment in some way so that if someone wants a concrete example, there is something to refer to. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com