From: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Alexander Lakhin <exclusion(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Fix inconsistencies for v12 |
Date: | 2019-05-28 06:59:28 |
Message-ID: | dffd9abe-be05-a1dd-64fa-5005163cec81@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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(at)entry=0x560a56254dc0, estate=estate(at)entry=0x560a563f9ae8,
> eflags=eflags(at)entry=0) at nodeForeignscan.c:227
> #2 0x0000560a5594e123 in ExecInitNode (node=node(at)entry=0x560a56254dc0,
> estate=estate(at)entry=0x560a563f9ae8,
> eflags=eflags(at)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
From | Date | Subject | |
---|---|---|---|
Next Message | Hubert Zhang | 2019-05-28 07:40:01 | Re: accounting for memory used for BufFile during hash joins |
Previous Message | Etsuro Fujita | 2019-05-28 06:40:48 | Re: BEFORE UPDATE trigger on postgres_fdw table not work |