From a0f6939e5b2ee8f78c813545f45d59a346d22e5f Mon Sep 17 00:00:00 2001 From: Etsuro Fujita Date: Thu, 8 Aug 2019 21:41:12 +0900 Subject: [PATCH v9 1/4] Remove dependency on estate->es_result_relation_info from FDW APIs. FDW APIs for executing a foreign table direct modification assumed that the FDW would obtain the target foreign table's ResultRelInfo from estate->es_result_relation_info of the passed-in ForeignScanState node, but the upcoming patch(es) to refactor partitioning-related code in nodeModifyTable.c will remove the es_result_relation_info variable. Revise BeginDirectModify()'s API to pass the ResultRelInfo explicitly, to remove the dependency on that variable from the FDW APIs. For ExecInitForeignScan() to efficiently get the ResultRelInfo to pass to BeginDirectModify(), add a field to ForeignScan that gives the index of the target foreign table in the list of the query result relations. Patch by Amit Langote, following a proposal by Andres Freund, reviewed by Andres Freund and me Discussion: https://postgr.es/m/20190718010911.l6xcdv6birtxiei4@alap3.anarazel.de --- contrib/postgres_fdw/postgres_fdw.c | 25 +++++++++++++++++++------ doc/src/sgml/fdwhandler.sgml | 8 ++++++-- src/backend/executor/nodeForeignscan.c | 14 ++++++++++---- src/backend/nodes/copyfuncs.c | 1 + src/backend/nodes/outfuncs.c | 1 + src/backend/nodes/readfuncs.c | 1 + src/backend/optimizer/plan/createplan.c | 2 ++ src/backend/optimizer/plan/setrefs.c | 15 +++++++++++++++ src/include/foreign/fdwapi.h | 1 + src/include/nodes/plannodes.h | 3 +++ 10 files changed, 59 insertions(+), 12 deletions(-) diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index bdc21b36d1..642deaf7cb 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -218,6 +218,7 @@ typedef struct PgFdwDirectModifyState int num_tuples; /* # of result tuples */ int next_tuple; /* index of next one to return */ Relation resultRel; /* relcache entry for the target relation */ + ResultRelInfo *resultRelInfo; /* ResultRelInfo for the target relation */ AttrNumber *attnoMap; /* array of attnums of input user columns */ AttrNumber ctidAttno; /* attnum of input ctid column */ AttrNumber oidAttno; /* attnum of input oid column */ @@ -361,7 +362,9 @@ static bool postgresPlanDirectModify(PlannerInfo *root, ModifyTable *plan, Index resultRelation, int subplan_index); -static void postgresBeginDirectModify(ForeignScanState *node, int eflags); +static void postgresBeginDirectModify(ForeignScanState *node, + ResultRelInfo *rinfo, + int eflags); static TupleTableSlot *postgresIterateDirectModify(ForeignScanState *node); static void postgresEndDirectModify(ForeignScanState *node); static void postgresExplainForeignScan(ForeignScanState *node, @@ -2319,6 +2322,11 @@ postgresPlanDirectModify(PlannerInfo *root, rebuild_fdw_scan_tlist(fscan, returningList); } + /* + * Set the index of the subplan result rel. + */ + fscan->resultRelIndex = subplan_index; + table_close(rel, NoLock); return true; } @@ -2328,7 +2336,9 @@ postgresPlanDirectModify(PlannerInfo *root, * Prepare a direct foreign table modification */ static void -postgresBeginDirectModify(ForeignScanState *node, int eflags) +postgresBeginDirectModify(ForeignScanState *node, + ResultRelInfo *rinfo, + int eflags) { ForeignScan *fsplan = (ForeignScan *) node->ss.ps.plan; EState *estate = node->ss.ps.state; @@ -2356,7 +2366,7 @@ 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; + rtindex = rinfo->ri_RangeTableIndex; rte = exec_rt_fetch(rtindex, estate); userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); @@ -2389,6 +2399,9 @@ postgresBeginDirectModify(ForeignScanState *node, int eflags) dmstate->rel = NULL; } + /* Save the ResultRelInfo for the target relation. */ + dmstate->resultRelInfo = rinfo; + /* Initialize state variable */ dmstate->num_tuples = -1; /* -1 means not set yet */ @@ -2451,7 +2464,7 @@ postgresIterateDirectModify(ForeignScanState *node) { PgFdwDirectModifyState *dmstate = (PgFdwDirectModifyState *) node->fdw_state; EState *estate = node->ss.ps.state; - ResultRelInfo *resultRelInfo = estate->es_result_relation_info; + ResultRelInfo *resultRelInfo = dmstate->resultRelInfo; /* * If this is the first call after Begin, execute the statement. @@ -4087,7 +4100,7 @@ get_returning_data(ForeignScanState *node) { PgFdwDirectModifyState *dmstate = (PgFdwDirectModifyState *) node->fdw_state; EState *estate = node->ss.ps.state; - ResultRelInfo *resultRelInfo = estate->es_result_relation_info; + ResultRelInfo *resultRelInfo = dmstate->resultRelInfo; TupleTableSlot *slot = node->ss.ss_ScanTupleSlot; TupleTableSlot *resultSlot; @@ -4234,7 +4247,7 @@ apply_returning_filter(PgFdwDirectModifyState *dmstate, TupleTableSlot *slot, EState *estate) { - ResultRelInfo *relInfo = estate->es_result_relation_info; + ResultRelInfo *relInfo = dmstate->resultRelInfo; TupleDesc resultTupType = RelationGetDescr(dmstate->resultRel); TupleTableSlot *resultSlot; Datum *values; diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml index 6587678af2..0aff958415 100644 --- a/doc/src/sgml/fdwhandler.sgml +++ b/doc/src/sgml/fdwhandler.sgml @@ -873,6 +873,7 @@ PlanDirectModify(PlannerInfo *root, void BeginDirectModify(ForeignScanState *node, + ResultRelInfo *rinfo, int eflags); @@ -886,6 +887,8 @@ BeginDirectModify(ForeignScanState *node, ForeignScanState node (in particular, from the underlying ForeignScan plan node, which contains any FDW-private information provided by PlanDirectModify). + In addition, the ResultRelInfo struct also + contains information about the target foreign table. eflags contains flag bits describing the executor's operating mode for this plan node. @@ -917,8 +920,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 + 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 52af1dac5c..84ef31ceef 100644 --- a/src/backend/executor/nodeForeignscan.c +++ b/src/backend/executor/nodeForeignscan.c @@ -221,12 +221,18 @@ 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) - fdwroutine->BeginDirectModify(scanstate, eflags); - else + if (node->operation == CMD_SELECT) fdwroutine->BeginForeignScan(scanstate, eflags); + else + { + ResultRelInfo *resultRelInfo; + + Assert(node->resultRelIndex >= 0); + resultRelInfo = &estate->es_result_relations[node->resultRelIndex]; + fdwroutine->BeginDirectModify(scanstate, resultRelInfo, eflags); + } return scanstate; } diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index a9b8b84b8f..a1094defb7 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 ac02e5ec8d..91af796a00 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 3f9ebc9044..d5514398f4 100644 --- a/src/backend/nodes/readfuncs.c +++ b/src/backend/nodes/readfuncs.c @@ -2013,6 +2013,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 8c8b4f8ed6..455740663b 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -5460,6 +5460,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 PlanDirectModify/setrefs.c, if needed */ + node->resultRelIndex = -1; return node; } diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c index f581c5f532..c16282b082 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -901,6 +901,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, @@ -1240,6 +1247,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/foreign/fdwapi.h b/src/include/foreign/fdwapi.h index 822686033e..adf39bc618 100644 --- a/src/include/foreign/fdwapi.h +++ b/src/include/foreign/fdwapi.h @@ -112,6 +112,7 @@ typedef bool (*PlanDirectModify_function) (PlannerInfo *root, int subplan_index); typedef void (*BeginDirectModify_function) (ForeignScanState *node, + ResultRelInfo *rinfo, int eflags); typedef TupleTableSlot *(*IterateDirectModify_function) (ForeignScanState *node); diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h index 477b4da192..f3f2699e90 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; /* ---------------- -- 2.16.5