From 385f337c9f39b21dca96ca4770552a10a6d5af24 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Tue, 8 Dec 2015 12:31:03 -0500 Subject: [PATCH] Allow foreign and custom joins to handle EvalPlanQual rechecks. Commit e7cb7ee14555cc9c5773e2c102efd6371f6f2005 provided basic infrastructure for allowing a foreign data wrapper or custom scan provider to replace a join of one or more tables with a scan. However, this infrastructure failed to take into account the need for possible EvalPlanQual rechecks, and ExecScanFetch would fail an assertion (or just overwrite memory) if such a check was attempted for a plan containing a pushed-down join. To fix, adjust the EPQ machinery to skip some processing steps when scanrelid == 0, making those the responsibility of scan's recheck method, which also has the responsibility in this case of correctly populating the relevant slot. To allow foreign scans to gain control in the right place to make use of this new facility, add a new, optional RecheckForeignScan method. Also, allow a foreign scan to have a child plan, which can be used to correctly populate the slot (or perhaps for something else, but this is the only use currently envisioned). KaiGai Kohei, reviewed by Robert Haas, Etsuro Fujita, and Kyotaro Horiguchi. --- contrib/file_fdw/file_fdw.c | 10 ++++-- contrib/postgres_fdw/postgres_fdw.c | 12 +++++-- doc/src/sgml/fdwhandler.sgml | 42 +++++++++++++++++++++-- src/backend/executor/execScan.c | 44 ++++++++++++++++++++++--- src/backend/executor/nodeForeignscan.c | 32 ++++++++++++++++++ src/backend/nodes/outfuncs.c | 1 + src/backend/optimizer/plan/createplan.c | 13 ++++++-- src/backend/optimizer/util/pathnode.c | 2 ++ src/include/foreign/fdwapi.h | 7 +++- src/include/nodes/relation.h | 1 + src/include/optimizer/pathnode.h | 1 + src/include/optimizer/planmain.h | 3 +- 12 files changed, 150 insertions(+), 18 deletions(-) diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index 5ce8f90cd8..83bbfa1212 100644 --- a/contrib/file_fdw/file_fdw.c +++ b/contrib/file_fdw/file_fdw.c @@ -121,7 +121,8 @@ static ForeignScan *fileGetForeignPlan(PlannerInfo *root, Oid foreigntableid, ForeignPath *best_path, List *tlist, - List *scan_clauses); + List *scan_clauses, + Plan *outer_plan); static void fileExplainForeignScan(ForeignScanState *node, ExplainState *es); static void fileBeginForeignScan(ForeignScanState *node, int eflags); static TupleTableSlot *fileIterateForeignScan(ForeignScanState *node); @@ -525,6 +526,7 @@ fileGetForeignPaths(PlannerInfo *root, total_cost, NIL, /* no pathkeys */ NULL, /* no outer rel either */ + NULL, /* no extra plan */ coptions)); /* @@ -544,7 +546,8 @@ fileGetForeignPlan(PlannerInfo *root, Oid foreigntableid, ForeignPath *best_path, List *tlist, - List *scan_clauses) + List *scan_clauses, + Plan *outer_plan) { Index scan_relid = baserel->relid; @@ -564,7 +567,8 @@ fileGetForeignPlan(PlannerInfo *root, NIL, /* no expressions to evaluate */ best_path->fdw_private, NIL, /* no custom tlist */ - NIL /* no remote quals */ ); + NIL, /* no remote quals */ + outer_plan); } /* diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index a6ba6723b1..9a014d4dba 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -214,7 +214,8 @@ static ForeignScan *postgresGetForeignPlan(PlannerInfo *root, Oid foreigntableid, ForeignPath *best_path, List *tlist, - List *scan_clauses); + List *scan_clauses, + Plan *outer_plan); static void postgresBeginForeignScan(ForeignScanState *node, int eflags); static TupleTableSlot *postgresIterateForeignScan(ForeignScanState *node); static void postgresReScanForeignScan(ForeignScanState *node); @@ -535,6 +536,7 @@ postgresGetForeignPaths(PlannerInfo *root, fpinfo->total_cost, NIL, /* no pathkeys */ NULL, /* no outer rel either */ + NULL, /* no extra plan */ NIL); /* no fdw_private list */ add_path(baserel, (Path *) path); @@ -589,6 +591,7 @@ postgresGetForeignPaths(PlannerInfo *root, total_cost, usable_pathkeys, NULL, + NULL, NIL)); } @@ -756,6 +759,7 @@ postgresGetForeignPaths(PlannerInfo *root, total_cost, NIL, /* no pathkeys */ param_info->ppi_req_outer, + NULL, NIL); /* no fdw_private list */ add_path(baserel, (Path *) path); } @@ -771,7 +775,8 @@ postgresGetForeignPlan(PlannerInfo *root, Oid foreigntableid, ForeignPath *best_path, List *tlist, - List *scan_clauses) + List *scan_clauses, + Plan *outer_plan) { PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) baserel->fdw_private; Index scan_relid = baserel->relid; @@ -915,7 +920,8 @@ postgresGetForeignPlan(PlannerInfo *root, params_list, fdw_private, NIL, /* no custom tlist */ - remote_exprs); + remote_exprs, + outer_plan); } /* diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml index 1533a6bf80..0090e2427e 100644 --- a/doc/src/sgml/fdwhandler.sgml +++ b/doc/src/sgml/fdwhandler.sgml @@ -168,7 +168,8 @@ GetForeignPlan (PlannerInfo *root, Oid foreigntableid, ForeignPath *best_path, List *tlist, - List *scan_clauses); + List *scan_clauses, + Plan *outer_plan); Create a ForeignScan plan node from the selected foreign @@ -765,6 +766,35 @@ RefetchForeignRow (EState *estate, See for more information. + + +bool +RecheckForeignScan (ForeignScanState *node, TupleTableSlot *slot); + + Recheck that a previously-returned tuple still matches the relevant + scan and join qualifiers, and possibly provide a modified version of + the tuple. For foreign data wrappers which do not perform join pushdown, + it will typically be more convenient to set this to NULL and + instead set fdw_recheck_quals appropriately. + When outer joins are pushed down, however, it isn't sufficient to + reapply the checks relevant to all the base tables to the result tuple, + even if all needed attributes are present, because failure to match some + qualifier might result in some attributes going to NULL, rather than in + no tuple being returned. RecheckForeignScan can recheck + qualifiers and return true if they are still satisfied and false + otherwise, but it can also store a replacement tuple into the supplied + slot. + + + + To implement join pushdown, a foreign data wrapper will typically + construct an alternative local join plan which is used only for + rechecks; this will become the outer subplan of the + ForeignScan. When a recheck is required, this subplan + can be executed and the resulting tuple can be stored in the slot. + This plan need not be efficient since no base table will return more + that one row; for example, it may implement all joins as nested loops. + @@ -1137,11 +1167,17 @@ GetForeignServerByName(const char *name, bool missing_ok); Any clauses removed from the plan node's qual list must instead be added - to fdw_recheck_quals in order to ensure correct behavior + to fdw_recheck_quals or rechecked by + RecheckForeignScan in order to ensure correct behavior at the READ COMMITTED isolation level. When a concurrent update occurs for some other table involved in the query, the executor may need to verify that all of the original quals are still satisfied for - the tuple, possibly against a different set of parameter values. + the tuple, possibly against a different set of parameter values. Using + fdw_recheck_quals is typically easier than implementing checks + inside RecheckForeignScan, but this method will be + insufficient when outer joins have been pushed down, since the join tuples + in that case might have some fields go to NULL without rejecting the + tuple entirely. diff --git a/src/backend/executor/execScan.c b/src/backend/executor/execScan.c index a96e826ba4..3faf7f9a77 100644 --- a/src/backend/executor/execScan.c +++ b/src/backend/executor/execScan.c @@ -49,8 +49,21 @@ ExecScanFetch(ScanState *node, */ Index scanrelid = ((Scan *) node->ps.plan)->scanrelid; - Assert(scanrelid > 0); - if (estate->es_epqTupleSet[scanrelid - 1]) + if (scanrelid == 0) + { + TupleTableSlot *slot = node->ss_ScanTupleSlot; + + /* + * This is a ForeignScan or CustomScan which has pushed down a + * join to the remote side. The recheck method is responsible not + * only for rechecking the scan/join quals but also for storing + * the correct tuple in the slot. + */ + if (!(*recheckMtd) (node, slot)) + ExecClearTuple(slot); /* would not be returned by scan */ + return slot; + } + else if (estate->es_epqTupleSet[scanrelid - 1]) { TupleTableSlot *slot = node->ss_ScanTupleSlot; @@ -347,8 +360,31 @@ ExecScanReScan(ScanState *node) { Index scanrelid = ((Scan *) node->ps.plan)->scanrelid; - Assert(scanrelid > 0); + if (scanrelid > 0) + estate->es_epqScanDone[scanrelid - 1] = false; + else + { + Bitmapset *relids; + int rtindex = -1; - estate->es_epqScanDone[scanrelid - 1] = false; + /* + * If an FDW or custom scan provider has replaced the join with a + * scan, there are multiple RTIs; reset the epqScanDone flag for + * all of them. + */ + if (IsA(node->ps.plan, ForeignScan)) + relids = ((ForeignScan *) node->ps.plan)->fs_relids; + else if (IsA(node->ps.plan, CustomScan)) + relids = ((CustomScan *) node->ps.plan)->custom_relids; + else + elog(ERROR, "unexpected scan node: %d", + (int) nodeTag(node->ps.plan)); + + while ((rtindex = bms_next_member(relids, rtindex)) >= 0) + { + Assert(rtindex > 0); + estate->es_epqScanDone[rtindex - 1] = false; + } + } } } diff --git a/src/backend/executor/nodeForeignscan.c b/src/backend/executor/nodeForeignscan.c index 6165e4a6cb..62959e3a64 100644 --- a/src/backend/executor/nodeForeignscan.c +++ b/src/backend/executor/nodeForeignscan.c @@ -73,6 +73,7 @@ ForeignNext(ForeignScanState *node) static bool ForeignRecheck(ForeignScanState *node, TupleTableSlot *slot) { + FdwRoutine *fdwroutine = node->fdwroutine; ExprContext *econtext; /* @@ -85,6 +86,18 @@ ForeignRecheck(ForeignScanState *node, TupleTableSlot *slot) ResetExprContext(econtext); + /* + * If an outer join is pushed down, RecheckForeignScan may need to store a + * different tuple in the slot, because a different set of columns may go + * to NULL upon recheck. Otherwise, it shouldn't need to change the slot + * contents, just return true or false to indicate whether the quals still + * pass. For simple cases, setting fdw_recheck_quals may be easier than + * providing this callback. + */ + if (fdwroutine->RecheckForeignScan && + !fdwroutine->RecheckForeignScan(node, slot)) + return false; + return ExecQual(node->fdw_recheck_quals, econtext, false); } @@ -205,6 +218,11 @@ ExecInitForeignScan(ForeignScan *node, EState *estate, int eflags) scanstate->fdwroutine = fdwroutine; scanstate->fdw_state = NULL; + /* Initialize any outer plan. */ + if (outerPlan(node)) + outerPlanState(scanstate) = + ExecInitNode(outerPlan(node), estate, eflags); + /* * Tell the FDW to initialize the scan. */ @@ -225,6 +243,10 @@ ExecEndForeignScan(ForeignScanState *node) /* Let the FDW shut down */ node->fdwroutine->EndForeignScan(node); + /* Shut down any outer plan. */ + if (outerPlanState(node)) + ExecEndNode(outerPlanState(node)); + /* Free the exprcontext */ ExecFreeExprContext(&node->ss.ps); @@ -246,7 +268,17 @@ ExecEndForeignScan(ForeignScanState *node) void ExecReScanForeignScan(ForeignScanState *node) { + PlanState *outerPlan = outerPlanState(node); + node->fdwroutine->ReScanForeignScan(node); + /* + * If chgParam of subnode is not null then plan will be re-scanned by + * first ExecProcNode. outerPlan may also be NULL, in which case there + * is nothing to rescan at all. + */ + if (outerPlan != NULL && outerPlan->chgParam == NULL) + ExecReScan(outerPlan); + ExecScanReScan(&node->ss); } diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index c709105d4f..f07c7933b1 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -1683,6 +1683,7 @@ _outForeignPath(StringInfo str, const ForeignPath *node) _outPathInfo(str, (const Path *) node); + WRITE_NODE_FIELD(fdw_outerpath); WRITE_NODE_FIELD(fdw_private); } diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index 411b36c418..32f903d8dc 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -2095,11 +2095,16 @@ create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path, Index scan_relid = rel->relid; Oid rel_oid = InvalidOid; Bitmapset *attrs_used = NULL; + Plan *outer_plan = NULL; ListCell *lc; int i; Assert(rel->fdwroutine != NULL); + /* transform the child path if any */ + if (best_path->fdw_outerpath) + outer_plan = create_plan_recurse(root, best_path->fdw_outerpath); + /* * If we're scanning a base relation, fetch its OID. (Irrelevant if * scanning a join relation.) @@ -2129,7 +2134,8 @@ create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path, */ scan_plan = rel->fdwroutine->GetForeignPlan(root, rel, rel_oid, best_path, - tlist, scan_clauses); + tlist, scan_clauses, + outer_plan); /* Copy cost data from Path to Plan; no need to make FDW do this */ copy_generic_path_info(&scan_plan->scan.plan, &best_path->path); @@ -3747,7 +3753,8 @@ make_foreignscan(List *qptlist, List *fdw_exprs, List *fdw_private, List *fdw_scan_tlist, - List *fdw_recheck_quals) + List *fdw_recheck_quals, + Plan *outer_plan) { ForeignScan *node = makeNode(ForeignScan); Plan *plan = &node->scan.plan; @@ -3755,7 +3762,7 @@ make_foreignscan(List *qptlist, /* cost will be filled in by create_foreignscan_plan */ plan->targetlist = qptlist; plan->qual = qpqual; - plan->lefttree = NULL; + plan->lefttree = outer_plan; plan->righttree = NULL; node->scan.scanrelid = scanrelid; /* fs_server will be filled in by create_foreignscan_plan */ diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c index 09c3244546..ec0910dbb4 100644 --- a/src/backend/optimizer/util/pathnode.c +++ b/src/backend/optimizer/util/pathnode.c @@ -1507,6 +1507,7 @@ create_foreignscan_path(PlannerInfo *root, RelOptInfo *rel, double rows, Cost startup_cost, Cost total_cost, List *pathkeys, Relids required_outer, + Path *fdw_outerpath, List *fdw_private) { ForeignPath *pathnode = makeNode(ForeignPath); @@ -1521,6 +1522,7 @@ create_foreignscan_path(PlannerInfo *root, RelOptInfo *rel, pathnode->path.total_cost = total_cost; pathnode->path.pathkeys = pathkeys; + pathnode->fdw_outerpath = fdw_outerpath; pathnode->fdw_private = fdw_private; return pathnode; diff --git a/src/include/foreign/fdwapi.h b/src/include/foreign/fdwapi.h index 69b48b4677..e9fdacd449 100644 --- a/src/include/foreign/fdwapi.h +++ b/src/include/foreign/fdwapi.h @@ -36,13 +36,17 @@ typedef ForeignScan *(*GetForeignPlan_function) (PlannerInfo *root, Oid foreigntableid, ForeignPath *best_path, List *tlist, - List *scan_clauses); + List *scan_clauses, + Plan *outer_plan); typedef void (*BeginForeignScan_function) (ForeignScanState *node, int eflags); typedef TupleTableSlot *(*IterateForeignScan_function) (ForeignScanState *node); +typedef bool (*RecheckForeignScan_function) (ForeignScanState *node, + TupleTableSlot *slot); + typedef void (*ReScanForeignScan_function) (ForeignScanState *node); typedef void (*EndForeignScan_function) (ForeignScanState *node); @@ -162,6 +166,7 @@ typedef struct FdwRoutine /* Functions for SELECT FOR UPDATE/SHARE row locking */ GetForeignRowMarkType_function GetForeignRowMarkType; RefetchForeignRow_function RefetchForeignRow; + RecheckForeignScan_function RecheckForeignScan; /* Support functions for EXPLAIN */ ExplainForeignScan_function ExplainForeignScan; diff --git a/src/include/nodes/relation.h b/src/include/nodes/relation.h index 6de07a1fbd..32321238ae 100644 --- a/src/include/nodes/relation.h +++ b/src/include/nodes/relation.h @@ -911,6 +911,7 @@ typedef struct TidPath typedef struct ForeignPath { Path path; + Path *fdw_outerpath; List *fdw_private; } ForeignPath; diff --git a/src/include/optimizer/pathnode.h b/src/include/optimizer/pathnode.h index eea75d0d1e..f41847f409 100644 --- a/src/include/optimizer/pathnode.h +++ b/src/include/optimizer/pathnode.h @@ -86,6 +86,7 @@ extern ForeignPath *create_foreignscan_path(PlannerInfo *root, RelOptInfo *rel, double rows, Cost startup_cost, Cost total_cost, List *pathkeys, Relids required_outer, + Path *fdw_outerpath, List *fdw_private); extern Relids calc_nestloop_required_outer(Path *outer_path, Path *inner_path); diff --git a/src/include/optimizer/planmain.h b/src/include/optimizer/planmain.h index 1fb850489f..f96e9ee505 100644 --- a/src/include/optimizer/planmain.h +++ b/src/include/optimizer/planmain.h @@ -45,7 +45,8 @@ extern SubqueryScan *make_subqueryscan(List *qptlist, List *qpqual, Index scanrelid, Plan *subplan); extern ForeignScan *make_foreignscan(List *qptlist, List *qpqual, Index scanrelid, List *fdw_exprs, List *fdw_private, - List *fdw_scan_tlist, List *fdw_recheck_quals); + List *fdw_scan_tlist, List *fdw_recheck_quals, + Plan *outer_plan); extern Append *make_append(List *appendplans, List *tlist); extern RecursiveUnion *make_recursive_union(List *tlist, Plan *lefttree, Plan *righttree, int wtParam, -- 2.40.0