Re: Fix inconsistencies for v12

On Wed, May 29, 2019 at 6:12 AM Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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