On 2019/05/30 18:51, Amit Kapila wrote:
> On Wed, May 29, 2019 at 6:12 AM Amit Langote wrote:
>> On 2019/05/28 20:26, Amit Kapila wrote:
>>> 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.
Maybe a good idea, but this will be the first time to mention postgres_fdw
in the core source code. If you think that's OK, how about the attached?
Thanks,
Amit
diff --git a/src/backend/executor/nodeModifyTable.c
b/src/backend/executor/nodeModifyTable.c
index a3c0e91543..da6da06115 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -2316,7 +2316,9 @@ 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 (see
+ * contrib/postgres_fdw/postgres_fdw.c: postgresBeginDirectModify()
+ * as one example.)
*/
saved_resultRelInfo = estate->es_result_relation_info;
