Re: Fix inconsistencies for v12 - Mailing list pgsql-hackers

From Amit Kapila
Subject Re: Fix inconsistencies for v12
Date
Msg-id CAA4eK1+yN5QgBJFr2Q_JGjoeOFu4ArnMDCgaoXu4DYJ2zP5NAQ@mail.gmail.com
Whole thread Raw
In response to Re: Fix inconsistencies for v12  (Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>)
Responses Re: Fix inconsistencies for v12
List pgsql-hackers
On Wed, May 29, 2019 at 6:12 AM Amit Langote
<Langote_Amit_f8@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



pgsql-hackers by date:

Previous
From: Amit Kapila
Date:
Subject: Re: Fix inconsistencies for v12
Next
From: Ashutosh Sharma
Date:
Subject: Re: Server crash due to assertion failure in CheckOpSlotCompatibility()