Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 5fc4c26

Browse filesBrowse 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 817588b commit 5fc4c26
Copy full SHA for 5fc4c26

File tree

Expand file treeCollapse file tree

13 files changed

+71
-13
lines changed
Filter options
Expand file treeCollapse file tree

13 files changed

+71
-13
lines changed

‎contrib/file_fdw/file_fdw.c

Copy file name to clipboardExpand all lines: contrib/file_fdw/file_fdw.c
+2-1Lines changed: 2 additions & 1 deletion
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

Copy file name to clipboardExpand all lines: contrib/postgres_fdw/postgres_fdw.c
+11-3Lines changed: 11 additions & 3 deletions
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

Copy file name to clipboardExpand all lines: doc/src/sgml/fdwhandler.sgml
+9Lines changed: 9 additions & 0 deletions
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

Copy file name to clipboardExpand all lines: src/backend/executor/nodeForeignscan.c
+17-2Lines changed: 17 additions & 2 deletions
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

Copy file name to clipboardExpand all lines: src/backend/nodes/copyfuncs.c
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -648,6 +648,7 @@ _copyForeignScan(const ForeignScan *from)
648648
COPY_NODE_FIELD(fdw_exprs);
649649
COPY_NODE_FIELD(fdw_private);
650650
COPY_NODE_FIELD(fdw_scan_tlist);
651+
COPY_NODE_FIELD(fdw_recheck_quals);
651652
COPY_BITMAPSET_FIELD(fs_relids);
652653
COPY_SCALAR_FIELD(fsSystemCol);
653654

‎src/backend/nodes/outfuncs.c

Copy file name to clipboardExpand all lines: src/backend/nodes/outfuncs.c
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -594,6 +594,7 @@ _outForeignScan(StringInfo str, const ForeignScan *node)
594594
WRITE_NODE_FIELD(fdw_exprs);
595595
WRITE_NODE_FIELD(fdw_private);
596596
WRITE_NODE_FIELD(fdw_scan_tlist);
597+
WRITE_NODE_FIELD(fdw_recheck_quals);
597598
WRITE_BITMAPSET_FIELD(fs_relids);
598599
WRITE_BOOL_FIELD(fsSystemCol);
599600
}

‎src/backend/nodes/readfuncs.c

Copy file name to clipboardExpand all lines: src/backend/nodes/readfuncs.c
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1798,6 +1798,7 @@ _readForeignScan(void)
17981798
READ_NODE_FIELD(fdw_exprs);
17991799
READ_NODE_FIELD(fdw_private);
18001800
READ_NODE_FIELD(fdw_scan_tlist);
1801+
READ_NODE_FIELD(fdw_recheck_quals);
18011802
READ_BITMAPSET_FIELD(fs_relids);
18021803
READ_BOOL_FIELD(fsSystemCol);
18031804

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

Copy file name to clipboardExpand all lines: src/backend/optimizer/plan/createplan.c
+6-1Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2153,6 +2153,9 @@ create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
21532153
replace_nestloop_params(root, (Node *) scan_plan->scan.plan.qual);
21542154
scan_plan->fdw_exprs = (List *)
21552155
replace_nestloop_params(root, (Node *) scan_plan->fdw_exprs);
2156+
scan_plan->fdw_recheck_quals = (List *)
2157+
replace_nestloop_params(root,
2158+
(Node *) scan_plan->fdw_recheck_quals);
21562159
}
21572160

21582161
/*
@@ -3738,7 +3741,8 @@ make_foreignscan(List *qptlist,
37383741
Index scanrelid,
37393742
List *fdw_exprs,
37403743
List *fdw_private,
3741-
List *fdw_scan_tlist)
3744+
List *fdw_scan_tlist,
3745+
List *fdw_recheck_quals)
37423746
{
37433747
ForeignScan *node = makeNode(ForeignScan);
37443748
Plan *plan = &node->scan.plan;
@@ -3754,6 +3758,7 @@ make_foreignscan(List *qptlist,
37543758
node->fdw_exprs = fdw_exprs;
37553759
node->fdw_private = fdw_private;
37563760
node->fdw_scan_tlist = fdw_scan_tlist;
3761+
node->fdw_recheck_quals = fdw_recheck_quals;
37573762
/* fs_relids will be filled in by create_foreignscan_plan */
37583763
node->fs_relids = NULL;
37593764
/* fsSystemCol will be filled in by create_foreignscan_plan */

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

Copy file name to clipboardExpand all lines: src/backend/optimizer/plan/setrefs.c
+3-1Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1133,13 +1133,15 @@ set_foreignscan_references(PlannerInfo *root,
11331133
}
11341134
else
11351135
{
1136-
/* Adjust tlist, qual, fdw_exprs in the standard way */
1136+
/* Adjust tlist, qual, fdw_exprs, etc. in the standard way */
11371137
fscan->scan.plan.targetlist =
11381138
fix_scan_list(root, fscan->scan.plan.targetlist, rtoffset);
11391139
fscan->scan.plan.qual =
11401140
fix_scan_list(root, fscan->scan.plan.qual, rtoffset);
11411141
fscan->fdw_exprs =
11421142
fix_scan_list(root, fscan->fdw_exprs, rtoffset);
1143+
fscan->fdw_recheck_quals =
1144+
fix_scan_list(root, fscan->fdw_recheck_quals, rtoffset);
11431145
}
11441146

11451147
/* Adjust fs_relids if needed */

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

Copy file name to clipboardExpand all lines: src/backend/optimizer/plan/subselect.c
+12-4Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2394,10 +2394,18 @@ finalize_plan(PlannerInfo *root, Plan *plan, Bitmapset *valid_params,
23942394
break;
23952395

23962396
case T_ForeignScan:
2397-
finalize_primnode((Node *) ((ForeignScan *) plan)->fdw_exprs,
2398-
&context);
2399-
/* We assume fdw_scan_tlist cannot contain Params */
2400-
context.paramids = bms_add_members(context.paramids, scan_params);
2397+
{
2398+
ForeignScan *fscan = (ForeignScan *) plan;
2399+
2400+
finalize_primnode((Node *) fscan->fdw_exprs,
2401+
&context);
2402+
finalize_primnode((Node *) fscan->fdw_recheck_quals,
2403+
&context);
2404+
2405+
/* We assume fdw_scan_tlist cannot contain Params */
2406+
context.paramids = bms_add_members(context.paramids,
2407+
scan_params);
2408+
}
24012409
break;
24022410

24032411
case T_CustomScan:

‎src/include/nodes/execnodes.h

Copy file name to clipboardExpand all lines: src/include/nodes/execnodes.h
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1579,6 +1579,7 @@ typedef struct WorkTableScanState
15791579
typedef struct ForeignScanState
15801580
{
15811581
ScanState ss; /* its first field is NodeTag */
1582+
List *fdw_recheck_quals; /* original quals not in ss.ps.qual */
15821583
/* use struct pointer to avoid including fdwapi.h here */
15831584
struct FdwRoutine *fdwroutine;
15841585
void *fdw_state; /* foreign-data wrapper can keep state here */

‎src/include/nodes/plannodes.h

Copy file name to clipboardExpand all lines: src/include/nodes/plannodes.h
+6Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -512,6 +512,11 @@ typedef struct WorkTableScan
512512
* fdw_scan_tlist is never actually executed; it just holds expression trees
513513
* describing what is in the scan tuple's columns.
514514
*
515+
* fdw_recheck_quals should contain any quals which the core system passed to
516+
* the FDW but which were not added to scan.plan.quals; that is, it should
517+
* contain the quals being checked remotely. This is needed for correct
518+
* behavior during EvalPlanQual rechecks.
519+
*
515520
* When the plan node represents a foreign join, scan.scanrelid is zero and
516521
* fs_relids must be consulted to identify the join relation. (fs_relids
517522
* is valid for simple scans as well, but will always match scan.scanrelid.)
@@ -524,6 +529,7 @@ typedef struct ForeignScan
524529
List *fdw_exprs; /* expressions that FDW may evaluate */
525530
List *fdw_private; /* private data for FDW */
526531
List *fdw_scan_tlist; /* optional tlist describing scan tuple */
532+
List *fdw_recheck_quals; /* original quals not in scan.plan.quals */
527533
Bitmapset *fs_relids; /* RTIs generated by this scan */
528534
bool fsSystemCol; /* true if any "system column" is needed */
529535
} ForeignScan;

‎src/include/optimizer/planmain.h

Copy file name to clipboardExpand all lines: src/include/optimizer/planmain.h
+1-1Lines changed: 1 addition & 1 deletion
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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.