Re: Fix inconsistencies for v12

On 2019/05/28 14:00, Alexander Lakhin wrote:
> 28.05.2019 2:05, Amit Kapila wrote:
>> ... If we read the comment atop ExecContextForcesOids
>> [1] before it was removed, it seems to indicate that the
>> initialization of es_result_relation_info for each subplan is for its
>> usage in ExecContextForcesOids.  I have run the regression tests with
>> the attached patch (which removes changing es_result_relation_info in
>> ExecInitModifyTable) and all the tests passed.  Do you have any
>> thoughts on this matter?
>
> I got a coredump with `make installcheck-world` (on postgres_fdw test):
> Core was generated by `postgres: law contrib_regression [local]
> UPDATE                               '.
> Program terminated with signal SIGSEGV, Segmentation fault.
> #0  0x00007ff1410ece98 in postgresBeginDirectModify
> (node=0x560a563fab30, eflags=0) at postgres_fdw.c:2363
> 2363            rtindex =
> estate->es_result_relation_info->ri_RangeTableIndex;
> (gdb) bt
> #0  0x00007ff1410ece98 in postgresBeginDirectModify
> (node=0x560a563fab30, eflags=0) at postgres_fdw.c:2363
> #1  0x0000560a55979e62 in ExecInitForeignScan
> (node=node@entry=0x560a56254dc0, estate=estate@entry=0x560a563f9ae8,
>     eflags=eflags@entry=0) at nodeForeignscan.c:227
> #2  0x0000560a5594e123 in ExecInitNode (node=node@entry=0x560a56254dc0,
> estate=estate@entry=0x560a563f9ae8,
>     eflags=eflags@entry=0) at execProcnode.c:277
> ...
> So It seems that this is not a dead code.

> ... So it seems that
> this comment at least diverged from the initial author's intent.
> With this in mind, I am inclined to just remove it.

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.

How about:

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; FDWs may depend on that.
      */
     saved_resultRelInfo = estate->es_result_relation_info;

Thanks,
Amit