8000 Allow FDWs to push down quals without breaking EvalPlanQual rechecks. · postgrespro/postgres_cluster@5fc4c26 · GitHub
[go: up one dir, main page]
Skip to content

Commit 5fc4c26

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 817588b commit 5fc4c26

File tree

13 files changed

+71
-13
lines changed

13 files changed

+71
-13
lines changed

‎contrib/file_fdw/file_fdw.c‎

Lines 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‎

Lines 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‎

Lines 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‎

Lines 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‎

Lines 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‎

Lines 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‎

Lines 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‎

Lines 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‎

Lines 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‎

Lines 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 *) (( A066 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:

0 commit comments

Comments
 (0)
0