On Tue, May 28, 2019 at 12:29 PM Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: > > 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. >
Can we ensure some way that only FDW's rely on it not any other part of the code? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
