On Tue, Dec 1, 2015 at 10:20 PM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp> wrote:
> One thing I can think of is that we can keep both the structure of a
> ForeignPath node and the API of create_foreignscan_path as-is. The latter
> is a good thing for FDW authors. And IIUC the patch you posted today, I
> think we could make create_foreignscan_plan a bit simpler too. Ie, in your
> patch, you modified that function as follows:
>
> @@ -2129,7 +2134,9 @@ 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);
> + outerPlan(scan_plan) = fdw_outerplan;
>
> I think that would be OK, but I think we would have to do a bit more here
> about the fdw_outerplan's targetlist and qual; I think that the targetlist
> needs to be changed to fdw_scan_tlist, as in the patch [1], and that it'd be
> better to change the qual to remote conditions, ie, quals not in the
> scan_plan's scan.plan.qual, to avoid duplicate evaluation of local
> conditions. (In the patch [1], I didn't do anything about the qual because
> the current postgres_fdw join pushdown patch assumes that all the the
> scan_plan's scan.plan.qual are pushed down.) Or, FDW authors might want to
> do something about fdw_recheck_quals for a foreign-join while creating the
> fdw_outerplan. So if we do that during GetForeignPlan, I think we could
> make create_foreignscan_plan a bit simpler, or provide flexibility to FDW
> authors.
It's certainly true that we need the alternative plan's tlist to match
that of the main plan; otherwise, it's going to be difficult for the
FDW to make use of that alternative subplan to fill its slot, which is
kinda the point of all this. However, I'm quite reluctant to
introduce code into create_foreignscan_plan() that forces the
subplan's tlist to match that of the main plan. For one thing, that
would likely foreclose the possibility of an FDW ever using the outer
plan for any purpose other than EPQ rechecks. It may be hard to
imagine what else you'd do with the outer plan as things are today,
but right now the two haves of the patch - letting FDWs have an outer
subplan, and providing them with a way of overriding the EPQ recheck
behavior - are technically independent. Putting tlist-altering
behavior into create_foreignscan_plan() ties those two things together
irrevocably.
Instead, I think we should go the opposite direction and pass the
outerplan to GetForeignPlan after all. I was lulled into a full sense
of security by the realization that every FDW that uses this feature
MUST want to do outerPlan(scan_plan) = fdw_outerplan. That's true,
but irrelevant. The point is that the FDW might want to do something
additional, like frob the outer plan's tlist, and it can't do that if
we don't pass it fdw_outerplan. So we should do that, after all.
Updated patch attached. This fixes a couple of whitespace issues that
were pointed out, also.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index 5ce8f90..83bbfa1 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 a6ba672..9a014d4 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 1533a6b..0090e24 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);
</programlisting>
Create a <structname>ForeignScan</> plan node from the selected foreign
@@ -765,6 +766,35 @@ RefetchForeignRow (EState *estate,
See <xref linkend="fdw-row-locking"> for more information.
</para>
+ <para>
+<programlisting>
+bool
+RecheckForeignScan (ForeignScanState *node, TupleTableSlot *slot);
+</programlisting>
+ 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 <literal>NULL</> and
+ instead set <structfield>fdw_recheck_quals</structfield> 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. <literal>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.
+ </para>
+
+ <para>
+ 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
+ <literal>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.
+ </para>
</sect2>
<sect2 id="fdw-callbacks-explain">
@@ -1137,11 +1167,17 @@ GetForeignServerByName(const char *name, bool missing_ok);
<para>
Any clauses removed from the plan node's qual list must instead be added
- to <literal>fdw_recheck_quals</> in order to ensure correct behavior
+ to <literal>fdw_recheck_quals</> or rechecked by
+ <literal>RecheckForeignScan</> in order to ensure correct behavior
at the <literal>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
+ <literal>fdw_recheck_quals</> is typically easier than implementing checks
+ inside <literal>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.
</para>
<para>
diff --git a/src/backend/executor/execScan.c b/src/backend/executor/execScan.c
index a96e826..3faf7f9 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 6165e4a..fdf88ff 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 (outerPlanState(scanstate))
+ 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 012c14b..aff27ea 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 411b36c..32f903d 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 09c3244..ec0910d 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 69b48b4..e9fdacd 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 9a0dd28..b072e1e 100644
--- a/src/include/nodes/relation.h
+++ b/src/include/nodes/relation.h
@@ -909,6 +909,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 f28b4e2..35e17e7 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 1fb8504..f96e9ee 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,
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
