On Wed, Oct 14, 2015 at 4:31 AM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp> wrote:
> Agreed.
>
> As KaiGai-san also pointed out before, I think we should address this in
> each of the following cases:
>
> 1) remote qual (scanrelid>0)
> 2) remote join (scanrelid==0)
>
> As for #1, I noticed that there is a bug in handling the same kind of FDW
> queries, which will be shown below. As you said, I think this should be
> addressed by rechecking the remote quals *locally*. (I thought another fix
> for this kind of bug before, though.) IIUC, I think this should be fixed
> separately from #2, as this is a bug not only in 9.5, but in back branches.
> Please find attached a patch.
+1 for doing something like this. However, I don't think we can
commit this to released branches, despite the fact that it's a bug
fix, because breaking third-party FDWs in a minor release seems
unfriendly. We might be able to slip it into 9.5, though, if we act
quickly.
A few review comments:
- nodeForeignscan.c now needs to #include "utils/memutils.h"
- I think it'd be safer for finalize_plan() not to try to shortcut
processing fdw_scan_quals.
- You forgot to update _readForeignScan.
- The documentation needs updating.
- I think we should use the name fdw_recheck_quals.
Here's an updated patch with those changes and some improvements to
the comments. Absent objections, I will commit it and back-patch to
9.5 only.
--
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 499f24f..5ce8f90 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -563,7 +563,8 @@ fileGetForeignPlan(PlannerInfo *root,
scan_relid,
NIL, /* no expressions to evaluate */
best_path->fdw_private,
- NIL /* no custom tlist */ );
+ NIL, /* no custom tlist */
+ NIL /* no remote quals */ );
}
/*
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index e4d799c..1902f1f 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -748,6 +748,7 @@ postgresGetForeignPlan(PlannerInfo *root,
Index scan_relid = baserel->relid;
List *fdw_private;
List *remote_conds = NIL;
+ List *remote_exprs = NIL;
List *local_exprs = NIL;
List *params_list = NIL;
List *retrieved_attrs;
@@ -769,8 +770,8 @@ postgresGetForeignPlan(PlannerInfo *root,
*
* This code must match "extract_actual_clauses(scan_clauses, false)"
* except for the additional decision about remote versus local execution.
- * Note however that we only strip the RestrictInfo nodes from the
- * local_exprs list, since appendWhereClause expects a list of
+ * Note however that we don't strip the RestrictInfo nodes from the
+ * remote_conds list, since appendWhereClause expects a list of
* RestrictInfos.
*/
foreach(lc, scan_clauses)
@@ -784,11 +785,17 @@ postgresGetForeignPlan(PlannerInfo *root,
continue;
if (list_member_ptr(fpinfo->remote_conds, rinfo))
+ {
remote_conds = lappend(remote_conds, rinfo);
+ remote_exprs = lappend(remote_exprs, rinfo->clause);
+ }
else if (list_member_ptr(fpinfo->local_conds, rinfo))
local_exprs = lappend(local_exprs, rinfo->clause);
else if (is_foreign_expr(root, baserel, rinfo->clause))
+ {
remote_conds = lappend(remote_conds, rinfo);
+ remote_exprs = lappend(remote_exprs, rinfo->clause);
+ }
else
local_exprs = lappend(local_exprs, rinfo->clause);
}
@@ -874,7 +881,8 @@ postgresGetForeignPlan(PlannerInfo *root,
scan_relid,
params_list,
fdw_private,
- NIL /* no custom tlist */ );
+ NIL, /* no custom tlist */
+ remote_exprs);
}
/*
diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
index 4c410c7..1533a6b 100644
--- a/doc/src/sgml/fdwhandler.sgml
+++ b/doc/src/sgml/fdwhandler.sgml
@@ -1136,6 +1136,15 @@ GetForeignServerByName(const char *name, bool missing_ok);
</para>
<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
+ 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.
+ </para>
+
+ <para>
Another <structname>ForeignScan</> field that can be filled by FDWs
is <structfield>fdw_scan_tlist</>, which describes the tuples returned by
the FDW for this plan node. For simple foreign table scans this can be
diff --git a/src/backend/executor/nodeForeignscan.c b/src/backend/executor/nodeForeignscan.c
index bb28a73..6165e4a 100644
--- a/src/backend/executor/nodeForeignscan.c
+++ b/src/backend/executor/nodeForeignscan.c
@@ -25,6 +25,7 @@
#include "executor/executor.h"
#include "executor/nodeForeignscan.h"
#include "foreign/fdwapi.h"
+#include "utils/memutils.h"
#include "utils/rel.h"
static TupleTableSlot *ForeignNext(ForeignScanState *node);
@@ -72,8 +73,19 @@ ForeignNext(ForeignScanState *node)
static bool
ForeignRecheck(ForeignScanState *node, TupleTableSlot *slot)
{
- /* There are no access-method-specific conditions to recheck. */
- return true;
+ ExprContext *econtext;
+
+ /*
+ * extract necessary information from foreign scan node
+ */
+ econtext = node->ss.ps.ps_ExprContext;
+
+ /* Does the tuple meet the remote qual condition? */
+ econtext->ecxt_scantuple = slot;
+
+ ResetExprContext(econtext);
+
+ return ExecQual(node->fdw_recheck_quals, econtext, false);
}
/* ----------------------------------------------------------------
@@ -135,6 +147,9 @@ ExecInitForeignScan(ForeignScan *node, EState *estate, int eflags)
scanstate->ss.ps.qual = (List *)
ExecInitExpr((Expr *) node->scan.plan.qual,
(PlanState *) scanstate);
+ scanstate->fdw_recheck_quals = (List *)
+ ExecInitExpr((Expr *) node->fdw_recheck_quals,
+ (PlanState *) scanstate);
/*
* tuple table initialization
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 0b4ab23..c176ff9 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -648,6 +648,7 @@ _copyForeignScan(const ForeignScan *from)
COPY_NODE_FIELD(fdw_exprs);
COPY_NODE_FIELD(fdw_private);
COPY_NODE_FIELD(fdw_scan_tlist);
+ COPY_NODE_FIELD(fdw_recheck_quals);
COPY_BITMAPSET_FIELD(fs_relids);
COPY_SCALAR_FIELD(fsSystemCol);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index df7f6e1..3e75cd1 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -594,6 +594,7 @@ _outForeignScan(StringInfo str, const ForeignScan *node)
WRITE_NODE_FIELD(fdw_exprs);
WRITE_NODE_FIELD(fdw_private);
WRITE_NODE_FIELD(fdw_scan_tlist);
+ WRITE_NODE_FIELD(fdw_recheck_quals);
WRITE_BITMAPSET_FIELD(fs_relids);
WRITE_BOOL_FIELD(fsSystemCol);
}
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 5802a73..94ba6dc 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1798,6 +1798,7 @@ _readForeignScan(void)
READ_NODE_FIELD(fdw_exprs);
READ_NODE_FIELD(fdw_private);
READ_NODE_FIELD(fdw_scan_tlist);
+ READ_NODE_FIELD(fdw_recheck_quals);
READ_BITMAPSET_FIELD(fs_relids);
READ_BOOL_FIELD(fsSystemCol);
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 0ee7392..791b64e 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -2153,6 +2153,9 @@ create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
replace_nestloop_params(root, (Node *) scan_plan->scan.plan.qual);
scan_plan->fdw_exprs = (List *)
replace_nestloop_params(root, (Node *) scan_plan->fdw_exprs);
+ scan_plan->fdw_recheck_quals = (List *)
+ replace_nestloop_params(root,
+ (Node *) scan_plan->fdw_recheck_quals);
}
/*
@@ -3738,7 +3741,8 @@ make_foreignscan(List *qptlist,
Index scanrelid,
List *fdw_exprs,
List *fdw_private,
- List *fdw_scan_tlist)
+ List *fdw_scan_tlist,
+ List *fdw_recheck_quals)
{
ForeignScan *node = makeNode(ForeignScan);
Plan *plan = &node->scan.plan;
@@ -3754,6 +3758,7 @@ make_foreignscan(List *qptlist,
node->fdw_exprs = fdw_exprs;
node->fdw_private = fdw_private;
node->fdw_scan_tlist = fdw_scan_tlist;
+ node->fdw_recheck_quals = fdw_recheck_quals;
/* fs_relids will be filled in by create_foreignscan_plan */
node->fs_relids = NULL;
/* fsSystemCol will be filled in by create_foreignscan_plan */
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 9392d61..8c6c571 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1133,13 +1133,15 @@ set_foreignscan_references(PlannerInfo *root,
}
else
{
- /* Adjust tlist, qual, fdw_exprs in the standard way */
+ /* Adjust tlist, qual, fdw_exprs, etc. in the standard way */
fscan->scan.plan.targetlist =
fix_scan_list(root, fscan->scan.plan.targetlist, rtoffset);
fscan->scan.plan.qual =
fix_scan_list(root, fscan->scan.plan.qual, rtoffset);
fscan->fdw_exprs =
fix_scan_list(root, fscan->fdw_exprs, rtoffset);
+ fscan->fdw_recheck_quals =
+ fix_scan_list(root, fscan->fdw_recheck_quals, rtoffset);
}
/* Adjust fs_relids if needed */
diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c
index 6b32f85..60b4ae1 100644
--- a/src/backend/optimizer/plan/subselect.c
+++ b/src/backend/optimizer/plan/subselect.c
@@ -2394,11 +2394,19 @@ finalize_plan(PlannerInfo *root, Plan *plan, Bitmapset *valid_params,
break;
case T_ForeignScan:
- finalize_primnode((Node *) ((ForeignScan *) plan)->fdw_exprs,
- &context);
- /* We assume fdw_scan_tlist cannot contain Params */
- context.paramids = bms_add_members(context.paramids, scan_params);
- break;
+ {
+ ForeignScan *fscan = (ForeignScan *) plan;
+
+ finalize_primnode((Node *) fscan->fdw_exprs,
+ &context);
+ finalize_primnode((Node *) fscan->fdw_recheck_quals,
+ &context);
+
+ /* We assume fdw_scan_tlist cannot contain Params */
+ context.paramids = bms_add_members(context.paramids,
+ scan_params);
+ break;
+ }
case T_CustomScan:
{
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index b6895f9..23670e1 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1579,6 +1579,7 @@ typedef struct WorkTableScanState
typedef struct ForeignScanState
{
ScanState ss; /* its first field is NodeTag */
+ List *fdw_recheck_quals; /* original quals not in ss.ps.qual */
/* use struct pointer to avoid including fdwapi.h here */
struct FdwRoutine *fdwroutine;
void *fdw_state; /* foreign-data wrapper can keep state here */
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 1f9213c..92fd8e4 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -512,6 +512,11 @@ typedef struct WorkTableScan
* fdw_scan_tlist is never actually executed; it just holds expression trees
* describing what is in the scan tuple's columns.
*
+ * fdw_recheck_quals should contain any quals which the core system passed to
+ * the FDW but which were not added to scan.plan.quals; that is, it should
+ * contain the quals being checked remotely. This is needed for correct
+ * behavior during EvalPlanQual rechecks.
+ *
* When the plan node represents a foreign join, scan.scanrelid is zero and
* fs_relids must be consulted to identify the join relation. (fs_relids
* is valid for simple scans as well, but will always match scan.scanrelid.)
@@ -524,6 +529,7 @@ typedef struct ForeignScan
List *fdw_exprs; /* expressions that FDW may evaluate */
List *fdw_private; /* private data for FDW */
List *fdw_scan_tlist; /* optional tlist describing scan tuple */
+ List *fdw_recheck_quals; /* original quals not in scan.plan.quals */
Bitmapset *fs_relids; /* RTIs generated by this scan */
bool fsSystemCol; /* true if any "system column" is needed */
} ForeignScan;
diff --git a/src/include/optimizer/planmain.h b/src/include/optimizer/planmain.h
index 52b077a..1fb8504 100644
--- a/src/include/optimizer/planmain.h
+++ b/src/include/optimizer/planmain.h
@@ -45,7 +45,7 @@ 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_scan_tlist, List *fdw_recheck_quals);
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
