Allow FDWs to push down quals without breaking EvalPlanQual rechecks.

This fixes a long-standing bug which was discovered while investigating
the interaction between the new join pushdown code and the EvalPlanQual
machinery: if a ForeignScan appears on the inner side of a paramaterized
nestloop, an EPQ recheck would re-return the original tuple even if
it no longer satisfied the pushed-down quals due to changed parameter
values.

This fix adds a new member to ForeignScan and ForeignScanState and a
new argument to make_foreignscan, and requires changes to FDWs which
push down quals to populate that new argument with a list of quals they
have chosen to push down.  Therefore, I'm only back-patching to 9.5,
even though the bug is not new in 9.5.

Etsuro Fujita, reviewed by me and by Kyotaro Horiguchi.

diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index 499f24ff2872e6b6fcecf22033af1bb9f6fa891b..5ce8f90cd8ba333beaa189260c6d7790167fb8f8 100644 (file)
--- 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 e4d799cecd541c777e21e7b48339b83c9e4b17b9..1902f1f4eae1f570a7d4752a0cc678b0399d1931 100644 (file)
--- 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 4c410c791689e4e958985c700047ed3f7d352cfd..1533a6bf80c3f888ab18707d1be6cabf1f337003 100644 (file)
--- a/doc/src/sgml/fdwhandler.sgml
+++ b/doc/src/sgml/fdwhandler.sgml
@@ -1135,6 +1135,15 @@ GetForeignServerByName(const char *name, bool missing_ok);
      evaluation of the <structfield>fdw_exprs</> expression tree.
     </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

diff --git a/src/backend/executor/nodeForeignscan.c b/src/backend/executor/nodeForeignscan.c
index bb28a7372d1be4a8dfc4cc9bd378d531af3a745d..6165e4a6cb432b1c32a6ea0104f3d14a68015dd4 100644 (file)
--- 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 73240406bafc29437ef74267d7cc7ee756c86b76..e19fee4c3181bc4094047380e09f2c66d637e8e4 100644 (file)
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -624,6 +624,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 991b4c217565728e254f220e01efa769e5e6d5d6..b39c772209bda6ac1029d862f18e1273a4c5c17a 100644 (file)
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -579,6 +579,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/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index f461586e08c5b3a2711eb55c003c26d2907388c7..224bf3d1476a450e2e0e16c2f3c864bb6f851ab7 100644 (file)
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -2117,6 +2117,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);
        }
 
        /*
@@ -3702,7 +3705,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;
@@ -3718,6 +3722,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 e6fbe6acde188409a3655b3939dae8d1363fc6b0..e1e1d7ab4bd9bc43c7bae337ff5153b631bf63b2 100644 (file)
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1127,13 +1127,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 f3038cdffda3ad9467935327df6c1cf7913798f1..00fb6376d97946e4ccf2ccabc18e681582776460 100644 (file)
--- a/src/backend/optimizer/plan/subselect.c
+++ b/src/backend/optimizer/plan/subselect.c
@@ -2371,10 +2371,18 @@ 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);
+                       {
+                               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 303fc3c1c77dca2f4c2abd51440c8614b1e88f61..0da5fb478adce32bd8f15249e284749ba75fa81b 100644 (file)
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1580,6 +1580,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 0654d0266cd6e7d17a7fb75c2834b6b6e793fbf4..5fd45b7b95fcf18b55b46a2a8a234ca7e00b77dc 100644 (file)
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -509,6 +509,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.)
@@ -521,6 +526,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 52b077a1b471516da3d4d4ada70d65afacb644f3..1fb850489fba014efdd772ad57847597a110c9bc 100644 (file)
--- 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,