On 2015/07/22 19:10, Etsuro Fujita wrote:
> While working on the issue "Foreign join pushdown vs EvalPlanQual", I
> happened to notice odd behaviors of late row locking in FDWs.
> I think the reason for that is because we don't check pushed-down quals
> inside an EPQ testing even if what was fetched by RefetchForeignRow was
> an updated version of the tuple rather than the same version previously
> obtained. So, to fix this, I'd like to propose that pushed-down quals
> be checked in ForeignRecheck.
Attached is a patch for that.
* I've modified ForeignRecheck so as to check pushed-down quals whether
doing late locking or early locking. I think we could probably make
ForeignRecheck do so only when doing late locking, but I'm not sure it's
worth complicating the code.
* I've made the above change only for simple foreign table scans that
have scanrelid > 0 and fdw_scan_tlist = NIL. As for simple foreign
table scans that have scanrelid > 0 and *fdw_scan_tlist is non-NIL*, I
think we are under discussion in another thread I started. Will update
as necessary.
* Sorry, I've not fully updated comments and docs yet. Will update.
I'd be happy if I could get feedback earlier.
Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
***************
*** 563,569 **** fileGetForeignPlan(PlannerInfo *root,
scan_relid,
NIL, /* no expressions to evaluate */
best_path->fdw_private,
! NIL /* no custom tlist */ );
}
/*
--- 563,570 ----
scan_relid,
NIL, /* no expressions to evaluate */
best_path->fdw_private,
! NIL, /* no custom tlist */
! NIL /* no pushed-down qual */ );
}
/*
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***************
*** 748,753 **** postgresGetForeignPlan(PlannerInfo *root,
--- 748,754 ----
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,776 **** 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
* RestrictInfos.
*/
foreach(lc, scan_clauses)
--- 770,777 ----
*
* This code must match "extract_actual_clauses(scan_clauses, false)"
* except for the additional decision about remote versus local execution.
! * 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,794 **** postgresGetForeignPlan(PlannerInfo *root,
--- 785,801 ----
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,880 **** postgresGetForeignPlan(PlannerInfo *root,
scan_relid,
params_list,
fdw_private,
! NIL /* no custom tlist */ );
}
/*
--- 881,888 ----
scan_relid,
params_list,
fdw_private,
! NIL, /* no custom tlist */
! remote_exprs);
}
/*
*** a/src/backend/executor/nodeForeignscan.c
--- b/src/backend/executor/nodeForeignscan.c
***************
*** 72,79 **** ForeignNext(ForeignScanState *node)
static bool
ForeignRecheck(ForeignScanState *node, TupleTableSlot *slot)
{
! /* There are no access-method-specific conditions to recheck. */
! return true;
}
/* ----------------------------------------------------------------
--- 72,90 ----
static bool
ForeignRecheck(ForeignScanState *node, TupleTableSlot *slot)
{
! ExprContext *econtext;
!
! /*
! * extract necessary information from foreign scan node
! */
! econtext = node->ss.ps.ps_ExprContext;
!
! /* Does the tuple meet the pushed-down-qual condition? */
! econtext->ecxt_scantuple = slot;
!
! ResetExprContext(econtext);
!
! return ExecQual(node->pushedDownQual, econtext, false);
}
/* ----------------------------------------------------------------
***************
*** 135,140 **** ExecInitForeignScan(ForeignScan *node, EState *estate, int eflags)
--- 146,154 ----
scanstate->ss.ps.qual = (List *)
ExecInitExpr((Expr *) node->scan.plan.qual,
(PlanState *) scanstate);
+ scanstate->pushedDownQual = (List *)
+ ExecInitExpr((Expr *) node->pushedDownQual,
+ (PlanState *) scanstate);
/*
* tuple table initialization
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
***************
*** 604,609 **** _copyForeignScan(const ForeignScan *from)
--- 604,610 ----
COPY_NODE_FIELD(fdw_private);
COPY_NODE_FIELD(fdw_scan_tlist);
COPY_BITMAPSET_FIELD(fs_relids);
+ COPY_NODE_FIELD(pushedDownQual);
COPY_SCALAR_FIELD(fsSystemCol);
return newnode;
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
***************
*** 570,575 **** _outForeignScan(StringInfo str, const ForeignScan *node)
--- 570,576 ----
WRITE_NODE_FIELD(fdw_private);
WRITE_NODE_FIELD(fdw_scan_tlist);
WRITE_BITMAPSET_FIELD(fs_relids);
+ WRITE_NODE_FIELD(pushedDownQual);
WRITE_BOOL_FIELD(fsSystemCol);
}
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***************
*** 2109,2114 **** create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
--- 2109,2116 ----
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->pushedDownQual = (List *)
+ replace_nestloop_params(root, (Node *) scan_plan->pushedDownQual);
}
/*
***************
*** 3692,3698 **** make_foreignscan(List *qptlist,
Index scanrelid,
List *fdw_exprs,
List *fdw_private,
! List *fdw_scan_tlist)
{
ForeignScan *node = makeNode(ForeignScan);
Plan *plan = &node->scan.plan;
--- 3694,3701 ----
Index scanrelid,
List *fdw_exprs,
List *fdw_private,
! List *fdw_scan_tlist,
! List *pushedDownQual)
{
ForeignScan *node = makeNode(ForeignScan);
Plan *plan = &node->scan.plan;
***************
*** 3710,3715 **** make_foreignscan(List *qptlist,
--- 3713,3719 ----
node->fdw_scan_tlist = fdw_scan_tlist;
/* fs_relids will be filled in by create_foreignscan_plan */
node->fs_relids = NULL;
+ node->pushedDownQual = pushedDownQual;
/* fsSystemCol will be filled in by create_foreignscan_plan */
node->fsSystemCol = false;
*** a/src/backend/optimizer/plan/setrefs.c
--- b/src/backend/optimizer/plan/setrefs.c
***************
*** 1132,1137 **** set_foreignscan_references(PlannerInfo *root,
--- 1132,1140 ----
fix_scan_list(root, fscan->scan.plan.qual, rtoffset);
fscan->fdw_exprs =
fix_scan_list(root, fscan->fdw_exprs, rtoffset);
+ /* pushedDownQual needs the same adjustments, too */
+ fscan->pushedDownQual =
+ fix_scan_list(root, fscan->pushedDownQual, rtoffset);
}
/* Adjust fs_relids if needed */
*** a/src/backend/optimizer/plan/subselect.c
--- b/src/backend/optimizer/plan/subselect.c
***************
*** 2368,2373 **** finalize_plan(PlannerInfo *root, Plan *plan, Bitmapset *valid_params,
--- 2368,2379 ----
case T_ForeignScan:
finalize_primnode((Node *) ((ForeignScan *) plan)->fdw_exprs,
&context);
+
+ /*
+ * we need not look at pushedDownQual, since it will have the same
+ * param references as fdw_exprs.
+ */
+
/* We assume fdw_scan_tlist cannot contain Params */
context.paramids = bms_add_members(context.paramids, scan_params);
break;
*** a/src/include/nodes/execnodes.h
--- b/src/include/nodes/execnodes.h
***************
*** 1571,1576 **** typedef struct WorkTableScanState
--- 1571,1577 ----
typedef struct ForeignScanState
{
ScanState ss; /* its first field is NodeTag */
+ List *pushedDownQual; /* list of pushed-down quals, if any */
/* use struct pointer to avoid including fdwapi.h here */
struct FdwRoutine *fdwroutine;
void *fdw_state; /* foreign-data wrapper can keep state here */
*** a/src/include/nodes/plannodes.h
--- b/src/include/nodes/plannodes.h
***************
*** 517,522 **** typedef struct ForeignScan
--- 517,523 ----
List *fdw_private; /* private data for FDW */
List *fdw_scan_tlist; /* optional tlist describing scan tuple */
Bitmapset *fs_relids; /* RTIs generated by this scan */
+ List *pushedDownQual; /* list of pushed-down quals, if any */
bool fsSystemCol; /* true if any "system column" is needed */
} ForeignScan;
*** a/src/include/optimizer/planmain.h
--- b/src/include/optimizer/planmain.h
***************
*** 45,51 **** 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);
extern Append *make_append(List *appendplans, List *tlist);
extern RecursiveUnion *make_recursive_union(List *tlist,
Plan *lefttree, Plan *righttree, int wtParam,
--- 45,51 ----
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 *pushedDownQual);
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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
