From 90ad2544925d5fba2fb4a6381c7154d0703b076d Mon Sep 17 00:00:00 2001 From: Etsuro Fujita Date: Thu, 8 Aug 2019 21:41:12 +0900 Subject: [PATCH v5 1/4] Revise how FDWs obtain result relation information For BeginDirectModify, which is called when ExecInitModifyTable initializes source plans, the only way currently to access information about the foreign table being modified is through ResultRelInfo for that table passed by setting EState.es_result_relation_info. This commit installs a new field in ForeignScan node to pass this information directly instead of through ResultRelInfo. This provides two benefits: 1. Reduce the reliance on es_result_relation_info (a query-global variable) being set correctly by the calling code, which can be bug-prone especially in the case of an inherited update. 2. Allows ResultRelInfos to be created at a later stage of ModifyTable execution instead of during ExecInitModifyTable. Amit Langote, Etsuro Fujita --- contrib/postgres_fdw/postgres_fdw.c | 26 ++++++++++++++++++-------- doc/src/sgml/fdwhandler.sgml | 10 ++++++---- src/backend/executor/nodeForeignscan.c | 5 ++++- src/backend/nodes/copyfuncs.c | 1 + src/backend/nodes/outfuncs.c | 1 + src/backend/nodes/readfuncs.c | 1 + src/backend/optimizer/plan/createplan.c | 8 ++++++++ src/backend/optimizer/plan/setrefs.c | 15 +++++++++++++++ src/include/nodes/plannodes.h | 3 +++ 9 files changed, 57 insertions(+), 13 deletions(-) diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index a31abce..13e256f 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -200,6 +200,9 @@ typedef struct PgFdwDirectModifyState Relation rel; /* relcache entry for the foreign table */ AttInMetadata *attinmeta; /* attribute datatype conversion metadata */ + int resultRelIndex; /* index of ResultRelInfo for the foreign table + * in EState.es_result_relations */ + /* extracted fdw_private data */ char *query; /* text of UPDATE/DELETE command */ bool has_returning; /* is there a RETURNING clause? */ @@ -446,11 +449,12 @@ static List *build_remote_returning(Index rtindex, Relation rel, List *returningList); static void rebuild_fdw_scan_tlist(ForeignScan *fscan, List *tlist); static void execute_dml_stmt(ForeignScanState *node); -static TupleTableSlot *get_returning_data(ForeignScanState *node); +static TupleTableSlot *get_returning_data(ForeignScanState *node, ResultRelInfo *resultRelInfo); static void init_returning_filter(PgFdwDirectModifyState *dmstate, List *fdw_scan_tlist, Index rtindex); static TupleTableSlot *apply_returning_filter(PgFdwDirectModifyState *dmstate, + ResultRelInfo *relInfo, TupleTableSlot *slot, EState *estate); static void prepare_query_params(PlanState *node, @@ -2331,6 +2335,7 @@ postgresBeginDirectModify(ForeignScanState *node, int eflags) { ForeignScan *fsplan = (ForeignScan *) node->ss.ps.plan; EState *estate = node->ss.ps.state; + List *resultRelations = estate->es_plannedstmt->resultRelations; PgFdwDirectModifyState *dmstate; Index rtindex; RangeTblEntry *rte; @@ -2355,7 +2360,9 @@ postgresBeginDirectModify(ForeignScanState *node, int eflags) * Identify which user to do the remote access as. This should match what * ExecCheckRTEPerms() does. */ - rtindex = estate->es_result_relation_info->ri_RangeTableIndex; + Assert(fsplan->resultRelIndex >= 0); + dmstate->resultRelIndex = fsplan->resultRelIndex; + rtindex = list_nth_int(resultRelations, fsplan->resultRelIndex); rte = exec_rt_fetch(rtindex, estate); userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); @@ -2450,7 +2457,10 @@ postgresIterateDirectModify(ForeignScanState *node) { PgFdwDirectModifyState *dmstate = (PgFdwDirectModifyState *) node->fdw_state; EState *estate = node->ss.ps.state; - ResultRelInfo *resultRelInfo = estate->es_result_relation_info; + ResultRelInfo *resultRelInfo = &estate->es_result_relations[dmstate->resultRelIndex]; + + /* The executor must have initialized the ResultRelInfo for us. */ + Assert(resultRelInfo != NULL); /* * If this is the first call after Begin, execute the statement. @@ -2482,7 +2492,7 @@ postgresIterateDirectModify(ForeignScanState *node) /* * Get the next RETURNING tuple. */ - return get_returning_data(node); + return get_returning_data(node, resultRelInfo); } /* @@ -4082,11 +4092,10 @@ execute_dml_stmt(ForeignScanState *node) * Get the result of a RETURNING clause. */ static TupleTableSlot * -get_returning_data(ForeignScanState *node) +get_returning_data(ForeignScanState *node, ResultRelInfo *resultRelInfo) { PgFdwDirectModifyState *dmstate = (PgFdwDirectModifyState *) node->fdw_state; EState *estate = node->ss.ps.state; - ResultRelInfo *resultRelInfo = estate->es_result_relation_info; TupleTableSlot *slot = node->ss.ss_ScanTupleSlot; TupleTableSlot *resultSlot; @@ -4141,7 +4150,8 @@ get_returning_data(ForeignScanState *node) if (dmstate->rel) resultSlot = slot; else - resultSlot = apply_returning_filter(dmstate, slot, estate); + resultSlot = apply_returning_filter(dmstate, resultRelInfo, slot, + estate); } dmstate->next_tuple++; @@ -4230,10 +4240,10 @@ init_returning_filter(PgFdwDirectModifyState *dmstate, */ static TupleTableSlot * apply_returning_filter(PgFdwDirectModifyState *dmstate, + ResultRelInfo *relInfo, TupleTableSlot *slot, EState *estate) { - ResultRelInfo *relInfo = estate->es_result_relation_info; TupleDesc resultTupType = RelationGetDescr(dmstate->resultRel); TupleTableSlot *resultSlot; Datum *values; diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml index 72fa127..1f6de9b 100644 --- a/doc/src/sgml/fdwhandler.sgml +++ b/doc/src/sgml/fdwhandler.sgml @@ -893,8 +893,9 @@ BeginDirectModify(ForeignScanState *node, its fdw_state field is still NULL. Information about the table to modify is accessible through the ForeignScanState node (in particular, from the underlying - ForeignScan plan node, which contains any FDW-private - information provided by PlanDirectModify). + ForeignScan plan node, which contains an integer field + giving the table's index in the query's list of result relations along with any + FDW-private information provided by PlanDirectModify. eflags contains flag bits describing the executor's operating mode for this plan node. @@ -926,8 +927,9 @@ IterateDirectModify(ForeignScanState *node); tuple table slot (the node's ScanTupleSlot should be used for this purpose). The data that was actually inserted, updated or deleted must be stored in the - es_result_relation_info->ri_projectReturning->pi_exprContext->ecxt_scantuple - of the node's EState. + ri_projectReturning->pi_exprContext->ecxt_scantuple + of the target foreign table's ResultRelInfo + obtained using the information passed to BeginDirectModify. Return NULL if no more rows are available. Note that this is called in a short-lived memory context that will be reset between invocations. Create a memory context in diff --git a/src/backend/executor/nodeForeignscan.c b/src/backend/executor/nodeForeignscan.c index 513471a..19433b3 100644 --- a/src/backend/executor/nodeForeignscan.c +++ b/src/backend/executor/nodeForeignscan.c @@ -221,10 +221,13 @@ ExecInitForeignScan(ForeignScan *node, EState *estate, int eflags) ExecInitNode(outerPlan(node), estate, eflags); /* - * Tell the FDW to initialize the scan. + * Tell the FDW to initialize the scan or the direct modification. */ if (node->operation != CMD_SELECT) + { + Assert(node->resultRelIndex >= 0); fdwroutine->BeginDirectModify(scanstate, eflags); + } else fdwroutine->BeginForeignScan(scanstate, eflags); diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 0409a40..de57744 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -761,6 +761,7 @@ _copyForeignScan(const ForeignScan *from) COPY_NODE_FIELD(fdw_recheck_quals); COPY_BITMAPSET_FIELD(fs_relids); COPY_SCALAR_FIELD(fsSystemCol); + COPY_SCALAR_FIELD(resultRelIndex); return newnode; } diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index e2f1775..15fd85a 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -698,6 +698,7 @@ _outForeignScan(StringInfo str, const ForeignScan *node) WRITE_NODE_FIELD(fdw_recheck_quals); WRITE_BITMAPSET_FIELD(fs_relids); WRITE_BOOL_FIELD(fsSystemCol); + WRITE_INT_FIELD(resultRelIndex); } static void diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c index 42050ab..4024a80 100644 --- a/src/backend/nodes/readfuncs.c +++ b/src/backend/nodes/readfuncs.c @@ -2017,6 +2017,7 @@ _readForeignScan(void) READ_NODE_FIELD(fdw_recheck_quals); READ_BITMAPSET_FIELD(fs_relids); READ_BOOL_FIELD(fsSystemCol); + READ_INT_FIELD(resultRelIndex); READ_DONE(); } diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index 99278ee..4e86249 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -5541,6 +5541,8 @@ make_foreignscan(List *qptlist, node->fs_relids = NULL; /* fsSystemCol will be filled in by create_foreignscan_plan */ node->fsSystemCol = false; + /* resultRelIndex will be set by make_modifytable(), if needed */ + node->resultRelIndex = -1; return node; } @@ -6900,7 +6902,13 @@ make_modifytable(PlannerInfo *root, !has_stored_generated_columns(subroot, rti)) direct_modify = fdwroutine->PlanDirectModify(subroot, node, rti, i); if (direct_modify) + { + ForeignScan *fscan = (ForeignScan *) list_nth(subplans, i); + + Assert(IsA(fscan, ForeignScan)); + fscan->resultRelIndex = i; direct_modify_plans = bms_add_member(direct_modify_plans, i); + } if (!direct_modify && fdwroutine != NULL && diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c index baefe0e..f3d1a12 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -904,6 +904,13 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset) rc->rti += rtoffset; rc->prti += rtoffset; } + /* + * Caution: Do not change the relative ordering of this loop + * and the statement below that adds the result relations to + * root->glob->resultRelations, because we need to use the + * current value of list_length(root->glob->resultRelations) + * in some plans. + */ foreach(l, splan->plans) { lfirst(l) = set_plan_refs(root, @@ -1243,6 +1250,14 @@ set_foreignscan_references(PlannerInfo *root, } fscan->fs_relids = offset_relid_set(fscan->fs_relids, rtoffset); + + /* + * Adjust resultRelIndex if it's valid (note that we are called before + * adding the RT indexes of ModifyTable result relations to the global + * list) + */ + if (fscan->resultRelIndex >= 0) + fscan->resultRelIndex += list_length(root->glob->resultRelations); } /* diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h index 83e0107..7314d2f 100644 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -620,6 +620,9 @@ typedef struct ForeignScan List *fdw_recheck_quals; /* original quals not in scan.plan.qual */ Bitmapset *fs_relids; /* RTIs generated by this scan */ bool fsSystemCol; /* true if any "system column" is needed */ + int resultRelIndex; /* index of foreign table in the list of query + * result relations for INSERT/UPDATE/DELETE; + * -1 for SELECT */ } ForeignScan; /* ---------------- -- 1.8.3.1