Skip to content

Commit 5043193

Browse files
robertmhaasrobertmhaas
committed
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.
1 parent 54e07be commit 5043193

File tree

12 files changed

+70
-13
lines changed

12 files changed

+70
-13
lines changed

‎contrib/file_fdw/file_fdw.c

+2-1
Original file line numberDiff line numberDiff line change
@@ -563,7 +563,8 @@ fileGetForeignPlan(PlannerInfo *root,
563563
scan_relid,
564564
NIL, /* no expressions to evaluate */
565565
best_path->fdw_private,
566-
NIL /* no custom tlist */ );
566+
NIL, /* no custom tlist */
567+
NIL /* no remote quals */ );
567568
}
568569

569570
/*

‎contrib/postgres_fdw/postgres_fdw.c

+11-3
Original file line numberDiff line numberDiff line change
@@ -748,6 +748,7 @@ postgresGetForeignPlan(PlannerInfo *root,
748748
Index scan_relid = baserel->relid;
749749
List *fdw_private;
750750
List *remote_conds = NIL;
751+
List *remote_exprs = NIL;
751752
List *local_exprs = NIL;
752753
List *params_list = NIL;
753754
List *retrieved_attrs;
@@ -769,8 +770,8 @@ postgresGetForeignPlan(PlannerInfo *root,
769770
*
770771
* This code must match "extract_actual_clauses(scan_clauses, false)"
771772
* except for the additional decision about remote versus local execution.
772-
* Note however that we only strip the RestrictInfo nodes from the
773-
* local_exprs list, since appendWhereClause expects a list of
773+
* Note however that we don't strip the RestrictInfo nodes from the
774+
* remote_conds list, since appendWhereClause expects a list of
774775
* RestrictInfos.
775776
*/
776777
foreach(lc, scan_clauses)
@@ -784,11 +785,17 @@ postgresGetForeignPlan(PlannerInfo *root,
784785
continue;
785786

786787
if (list_member_ptr(fpinfo->remote_conds, rinfo))
788+
{
787789
remote_conds = lappend(remote_conds, rinfo);
790+
remote_exprs = lappend(remote_exprs, rinfo->clause);
791+
}
788792
else if (list_member_ptr(fpinfo->local_conds, rinfo))
789793
local_exprs = lappend(local_exprs, rinfo->clause);
790794
else if (is_foreign_expr(root, baserel, rinfo->clause))
795+
{
791796
remote_conds = lappend(remote_conds, rinfo);
797+
remote_exprs = lappend(remote_exprs, rinfo->clause);
798+
}
792799
else
793800
local_exprs = lappend(local_exprs, rinfo->clause);
794801
}
@@ -874,7 +881,8 @@ postgresGetForeignPlan(PlannerInfo *root,
874881
scan_relid,
875882
params_list,
876883
fdw_private,
877-
NIL /* no custom tlist */ );
884+
NIL, /* no custom tlist */
885+
remote_exprs);
878886
}
879887

880888
/*

‎doc/src/sgml/fdwhandler.sgml

+9
Original file line numberDiff line numberDiff line change
@@ -1135,6 +1135,15 @@ GetForeignServerByName(const char *name, bool missing_ok);
11351135
evaluation of the <structfield>fdw_exprs</> expression tree.
11361136
</para>
11371137

1138+
<para>
1139+
Any clauses removed from the plan node's qual list must instead be added
1140+
to <literal>fdw_recheck_quals</> in order to ensure correct behavior
1141+
at the <literal>READ COMMITTED</> isolation level. When a concurrent
1142+
update occurs for some other table involved in the query, the executor
1143+
may need to verify that all of the original quals are still satisfied for
1144+
the tuple, possibly against a different set of parameter values.
1145+
</para>
1146+
11381147
<para>
11391148
Another <structname>ForeignScan</> field that can be filled by FDWs
11401149
is <structfield>fdw_scan_tlist</>, which describes the tuples returned by

‎src/backend/executor/nodeForeignscan.c

+17-2
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#include "executor/executor.h"
2626
#include "executor/nodeForeignscan.h"
2727
#include "foreign/fdwapi.h"
28+
#include "utils/memutils.h"
2829
#include "utils/rel.h"
2930

3031
static TupleTableSlot *ForeignNext(ForeignScanState *node);
@@ -72,8 +73,19 @@ ForeignNext(ForeignScanState *node)
7273
static bool
7374
ForeignRecheck(ForeignScanState *node, TupleTableSlot *slot)
7475
{
75-
/* There are no access-method-specific conditions to recheck. */
76-
return true;
76+
ExprContext *econtext;
77+
78+
/*
79+
* extract necessary information from foreign scan node
80+
*/
81+
econtext = node->ss.ps.ps_ExprContext;
82+
83+
/* Does the tuple meet the remote qual condition? */
84+
econtext->ecxt_scantuple = slot;
85+
86+
ResetExprContext(econtext);
87+
88+
return ExecQual(node->fdw_recheck_quals, econtext, false);
7789
}
7890

7991
/* ----------------------------------------------------------------
@@ -135,6 +147,9 @@ ExecInitForeignScan(ForeignScan *node, EState *estate, int eflags)
135147
scanstate->ss.ps.qual = (List *)
136148
ExecInitExpr((Expr *) node->scan.plan.qual,
137149
(PlanState *) scanstate);
150+
scanstate->fdw_recheck_quals = (List *)
151+
ExecInitExpr((Expr *) node->fdw_recheck_quals,
152+
(PlanState *) scanstate);
138153

139154
/*
140155
* tuple table initialization

‎src/backend/nodes/copyfuncs.c

+1
Original file line numberDiff line numberDiff line change
@@ -624,6 +624,7 @@ _copyForeignScan(const ForeignScan *from)
624624
COPY_NODE_FIELD(fdw_exprs);
625625
COPY_NODE_FIELD(fdw_private);
626626
COPY_NODE_FIELD(fdw_scan_tlist);
627+
COPY_NODE_FIELD(fdw_recheck_quals);
627628
COPY_BITMAPSET_FIELD(fs_relids);
628629
COPY_SCALAR_FIELD(fsSystemCol);
629630

‎src/backend/nodes/outfuncs.c

+1
Original file line numberDiff line numberDiff line change
@@ -579,6 +579,7 @@ _outForeignScan(StringInfo str, const ForeignScan *node)
579579
WRITE_NODE_FIELD(fdw_exprs);
580580
WRITE_NODE_FIELD(fdw_private);
581581
WRITE_NODE_FIELD(fdw_scan_tlist);
582+
WRITE_NODE_FIELD(fdw_recheck_quals);
582583
WRITE_BITMAPSET_FIELD(fs_relids);
583584
WRITE_BOOL_FIELD(fsSystemCol);
584585
}

‎src/backend/optimizer/plan/createplan.c

+6-1
Original file line numberDiff line numberDiff line change
@@ -2117,6 +2117,9 @@ create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
21172117
replace_nestloop_params(root, (Node *) scan_plan->scan.plan.qual);
21182118
scan_plan->fdw_exprs = (List *)
21192119
replace_nestloop_params(root, (Node *) scan_plan->fdw_exprs);
2120+
scan_plan->fdw_recheck_quals = (List *)
2121+
replace_nestloop_params(root,
2122+
(Node *) scan_plan->fdw_recheck_quals);
21202123
}
21212124

21222125
/*
@@ -3702,7 +3705,8 @@ make_foreignscan(List *qptlist,
37023705
Index scanrelid,
37033706
List *fdw_exprs,
37043707
List *fdw_private,
3705-
List *fdw_scan_tlist)
3708+
List *fdw_scan_tlist,
3709+
List *fdw_recheck_quals)
37063710
{
37073711
ForeignScan *node = makeNode(ForeignScan);
37083712
Plan *plan = &node->scan.plan;
@@ -3718,6 +3722,7 @@ make_foreignscan(List *qptlist,
37183722
node->fdw_exprs = fdw_exprs;
37193723
node->fdw_private = fdw_private;
37203724
node->fdw_scan_tlist = fdw_scan_tlist;
3725+
node->fdw_recheck_quals = fdw_recheck_quals;
37213726
/* fs_relids will be filled in by create_foreignscan_plan */
37223727
node->fs_relids = NULL;
37233728
/* fsSystemCol will be filled in by create_foreignscan_plan */

‎src/backend/optimizer/plan/setrefs.c

+3-1
Original file line numberDiff line numberDiff line change
@@ -1127,13 +1127,15 @@ set_foreignscan_references(PlannerInfo *root,
11271127
}
11281128
else
11291129
{
1130-
/* Adjust tlist, qual, fdw_exprs in the standard way */
1130+
/* Adjust tlist, qual, fdw_exprs, etc. in the standard way */
11311131
fscan->scan.plan.targetlist =
11321132
fix_scan_list(root, fscan->scan.plan.targetlist, rtoffset);
11331133
fscan->scan.plan.qual =
11341134
fix_scan_list(root, fscan->scan.plan.qual, rtoffset);
11351135
fscan->fdw_exprs =
11361136
fix_scan_list(root, fscan->fdw_exprs, rtoffset);
1137+
fscan->fdw_recheck_quals =
1138+
fix_scan_list(root, fscan->fdw_recheck_quals, rtoffset);
11371139
}
11381140

11391141
/* Adjust fs_relids if needed */

‎src/backend/optimizer/plan/subselect.c

+12-4
Original file line numberDiff line numberDiff line change
@@ -2371,10 +2371,18 @@ finalize_plan(PlannerInfo *root, Plan *plan, Bitmapset *valid_params,
23712371
break;
23722372

23732373
case T_ForeignScan:
2374-
finalize_primnode((Node *) ((ForeignScan *) plan)->fdw_exprs,
2375-
&context);
2376-
/* We assume fdw_scan_tlist cannot contain Params */
2377-
context.paramids = bms_add_members(context.paramids, scan_params);
2374+
{
2375+
ForeignScan *fscan = (ForeignScan *) plan;
2376+
2377+
finalize_primnode((Node *) fscan->fdw_exprs,
2378+
&context);
2379+
finalize_primnode((Node *) fscan->fdw_recheck_quals,
2380+
&context);
2381+
2382+
/* We assume fdw_scan_tlist cannot contain Params */
2383+
context.paramids = bms_add_members(context.paramids,
2384+
scan_params);
2385+
}
23782386
break;
23792387

23802388
case T_CustomScan:

‎src/include/nodes/execnodes.h

+1
Original file line numberDiff line numberDiff line change
@@ -1580,6 +1580,7 @@ typedef struct WorkTableScanState
15801580
typedef struct ForeignScanState
15811581
{
15821582
ScanState ss; /* its first field is NodeTag */
1583+
List *fdw_recheck_quals; /* original quals not in ss.ps.qual */
15831584
/* use struct pointer to avoid including fdwapi.h here */
15841585
struct FdwRoutine *fdwroutine;
15851586
void *fdw_state; /* foreign-data wrapper can keep state here */

‎src/include/nodes/plannodes.h

+6
Original file line numberDiff line numberDiff line change
@@ -509,6 +509,11 @@ typedef struct WorkTableScan
509509
* fdw_scan_tlist is never actually executed; it just holds expression trees
510510
* describing what is in the scan tuple's columns.
511511
*
512+
* fdw_recheck_quals should contain any quals which the core system passed to
513+
* the FDW but which were not added to scan.plan.quals; that is, it should
514+
* contain the quals being checked remotely. This is needed for correct
515+
* behavior during EvalPlanQual rechecks.
516+
*
512517
* When the plan node represents a foreign join, scan.scanrelid is zero and
513518
* fs_relids must be consulted to identify the join relation. (fs_relids
514519
* is valid for simple scans as well, but will always match scan.scanrelid.)
@@ -521,6 +526,7 @@ typedef struct ForeignScan
521526
List *fdw_exprs; /* expressions that FDW may evaluate */
522527
List *fdw_private; /* private data for FDW */
523528
List *fdw_scan_tlist; /* optional tlist describing scan tuple */
529+
List *fdw_recheck_quals; /* original quals not in scan.plan.quals */
524530
Bitmapset *fs_relids; /* RTIs generated by this scan */
525531
bool fsSystemCol; /* true if any "system column" is needed */
526532
} ForeignScan;

‎src/include/optimizer/planmain.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ extern SubqueryScan *make_subqueryscan(List *qptlist, List *qpqual,
4545
Index scanrelid, Plan *subplan);
4646
extern ForeignScan *make_foreignscan(List *qptlist, List *qpqual,
4747
Index scanrelid, List *fdw_exprs, List *fdw_private,
48-
List *fdw_scan_tlist);
48+
List *fdw_scan_tlist, List *fdw_recheck_quals);
4949
extern Append *make_append(List *appendplans, List *tlist);
5050
extern RecursiveUnion *make_recursive_union(List *tlist,
5151
Plan *lefttree, Plan *righttree, int wtParam,

0 commit comments

Comments
 (0)