On 2015/10/16 19:03, Kouhei Kaigai wrote:
*** 48,59 **** ExecScanFetch(ScanState *node,
+ /*
+ * Execute recheck plan and get the next tuple if foreign join.
+ */
+ if (scanrelid == 0)
+ {
+ (*recheckMtd) (node, slot);
+ return slot;
+ }
Ensure the slot is empty if recheckMtd returned false, as base relation
case doing so.
Fixed.
*** 347,352 **** ExecScanReScan(ScanState *node)
{
Index scanrelid = ((Scan *) node->ps.plan)->scanrelid;
+ if (scanrelid == 0)
+ return; /* nothing to do */
+
Assert(scanrelid > 0);
estate->es_epqScanDone[scanrelid - 1] = false;
Why nothing to do?
Base relations managed by ForeignScan are tracked in fs_relids bitmap.
I think the estate->es_epqScanDone flag should be initialized when we do
ExecScanReSacn for each of the component ForeignScanState nodes in the
local join execution plan state tree.
As you introduced a few days before, if ForeignScan has parametalized
remote join, EPQ slot contains invalid tuples based on old outer tuple.
Maybe my explanation was not enough, but I haven't said such a thing.
The problem in that case is that just returning the previously-returned
foeign-join tuple would produce an incorrect result if an outer tuple to
be joined has changed due to a concurrent transaction, as explained
upthread. (I think that the EPQ slots would contain valid tuples.)
Attached is an updated version of the patch.
Other changes:
* remove unnecessary memory-context handling for the foreign-join case
in ForeignRecheck
* revise code a bit and add a bit more comments
Thanks for the comments!
Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
***************
*** 525,530 **** fileGetForeignPaths(PlannerInfo *root,
--- 525,531 ----
total_cost,
NIL, /* no pathkeys */
NULL, /* no outer rel either */
+ NULL, /* no alternative path */
coptions));
/*
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***************
*** 560,565 **** postgresGetForeignPaths(PlannerInfo *root,
--- 560,566 ----
fpinfo->total_cost,
NIL, /* no pathkeys */
NULL, /* no outer rel either */
+ NULL, /* no alternative path */
NIL); /* no fdw_private list */
add_path(baserel, (Path *) path);
***************
*** 727,732 **** postgresGetForeignPaths(PlannerInfo *root,
--- 728,734 ----
total_cost,
NIL, /* no pathkeys */
param_info->ppi_req_outer,
+ NULL, /* no alternative path */
NIL); /* no fdw_private list */
add_path(baserel, (Path *) path);
}
*** a/src/backend/executor/execScan.c
--- b/src/backend/executor/execScan.c
***************
*** 48,59 **** ExecScanFetch(ScanState *node,
* conditions.
*/
Index scanrelid = ((Scan *) node->ps.plan)->scanrelid;
Assert(scanrelid > 0);
if (estate->es_epqTupleSet[scanrelid - 1])
{
- TupleTableSlot *slot = node->ss_ScanTupleSlot;
-
/* Return empty slot if we already returned a tuple */
if (estate->es_epqScanDone[scanrelid - 1])
return ExecClearTuple(slot);
--- 48,67 ----
* conditions.
*/
Index scanrelid = ((Scan *) node->ps.plan)->scanrelid;
+ TupleTableSlot *slot = node->ss_ScanTupleSlot;
+
+ if (scanrelid == 0)
+ {
+ /* Execute recheck plan and store result in the slot */
+ if (!(*recheckMtd) (node, slot))
+ ExecClearTuple(slot); /* would not be returned by scan */
+
+ return slot;
+ }
Assert(scanrelid > 0);
if (estate->es_epqTupleSet[scanrelid - 1])
{
/* Return empty slot if we already returned a tuple */
if (estate->es_epqScanDone[scanrelid - 1])
return ExecClearTuple(slot);
***************
*** 347,352 **** ExecScanReScan(ScanState *node)
--- 355,363 ----
{
Index scanrelid = ((Scan *) node->ps.plan)->scanrelid;
+ if (scanrelid == 0)
+ return; /* nothing to do */
+
Assert(scanrelid > 0);
estate->es_epqScanDone[scanrelid - 1] = false;
*** a/src/backend/executor/nodeForeignscan.c
--- b/src/backend/executor/nodeForeignscan.c
***************
*** 24,29 ****
--- 24,30 ----
#include "executor/executor.h"
#include "executor/nodeForeignscan.h"
+ #include "executor/tuptable.h"
#include "foreign/fdwapi.h"
#include "utils/memutils.h"
#include "utils/rel.h"
***************
*** 73,80 **** ForeignNext(ForeignScanState *node)
--- 74,99 ----
static bool
ForeignRecheck(ForeignScanState *node, TupleTableSlot *slot)
{
+ Index scanrelid = ((Scan *) node->ss.ps.plan)->scanrelid;
ExprContext *econtext;
+ if (scanrelid == 0)
+ {
+ TupleTableSlot *result;
+
+ Assert(node->fdw_recheck_plan != NULL);
+
+ /* Execute recheck plan */
+ result = ExecProcNode(node->fdw_recheck_plan);
+ if (TupIsNull(result))
+ return false;
+
+ /* Store result in the given slot */
+ ExecCopySlot(slot, result);
+
+ return true;
+ }
+
/*
* extract necessary information from foreign scan node
*/
***************
*** 200,205 **** ExecInitForeignScan(ForeignScan *node, EState *estate, int eflags)
--- 219,230 ----
ExecAssignScanProjectionInfoWithVarno(&scanstate->ss, tlistvarno);
/*
+ * Initialize recheck plan.
+ */
+ scanstate->fdw_recheck_plan = ExecInitNode(node->fdw_recheck_plan,
+ estate, eflags);
+
+ /*
* Initialize FDW-related state.
*/
scanstate->fdwroutine = fdwroutine;
***************
*** 235,240 **** ExecEndForeignScan(ForeignScanState *node)
--- 260,268 ----
/* close the relation. */
if (node->ss.ss_currentRelation)
ExecCloseScanRelation(node->ss.ss_currentRelation);
+
+ /* shut down recheck plan. */
+ ExecEndNode(node->fdw_recheck_plan);
}
/* ----------------------------------------------------------------
***************
*** 246,252 **** ExecEndForeignScan(ForeignScanState *node)
--- 274,301 ----
void
ExecReScanForeignScan(ForeignScanState *node)
{
+ Index scanrelid = ((Scan *) node->ss.ps.plan)->scanrelid;
+
node->fdwroutine->ReScanForeignScan(node);
ExecScanReScan(&node->ss);
+
+ if (scanrelid == 0)
+ {
+ Assert(node->fdw_recheck_plan != NULL);
+
+ /*
+ * set chgParam for recheck plan
+ */
+ if (((PlanState *) node)->chgParam != NULL)
+ UpdateChangedParamSet(node->fdw_recheck_plan,
+ ((PlanState *) node)->chgParam);
+
+ /*
+ * if chgParam of recheck plan is not null then the plan will be
+ * re-scanned by first ExecProcNode.
+ */
+ if (node->fdw_recheck_plan->chgParam == NULL)
+ ExecReScan(node->fdw_recheck_plan);
+ }
}
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
***************
*** 648,653 **** _copyForeignScan(const ForeignScan *from)
--- 648,654 ----
COPY_NODE_FIELD(fdw_exprs);
COPY_NODE_FIELD(fdw_private);
COPY_NODE_FIELD(fdw_scan_tlist);
+ COPY_NODE_FIELD(fdw_recheck_plan);
COPY_NODE_FIELD(fdw_recheck_quals);
COPY_BITMAPSET_FIELD(fs_relids);
COPY_SCALAR_FIELD(fsSystemCol);
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
***************
*** 594,599 **** _outForeignScan(StringInfo str, const ForeignScan *node)
--- 594,600 ----
WRITE_NODE_FIELD(fdw_exprs);
WRITE_NODE_FIELD(fdw_private);
WRITE_NODE_FIELD(fdw_scan_tlist);
+ WRITE_NODE_FIELD(fdw_recheck_plan);
WRITE_NODE_FIELD(fdw_recheck_quals);
WRITE_BITMAPSET_FIELD(fs_relids);
WRITE_BOOL_FIELD(fsSystemCol);
*** a/src/backend/nodes/readfuncs.c
--- b/src/backend/nodes/readfuncs.c
***************
*** 1798,1803 **** _readForeignScan(void)
--- 1798,1804 ----
READ_NODE_FIELD(fdw_exprs);
READ_NODE_FIELD(fdw_private);
READ_NODE_FIELD(fdw_scan_tlist);
+ READ_NODE_FIELD(fdw_recheck_plan);
READ_NODE_FIELD(fdw_recheck_quals);
READ_BITMAPSET_FIELD(fs_relids);
READ_BOOL_FIELD(fsSystemCol);
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***************
*** 2141,2146 **** create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
--- 2141,2157 ----
scan_plan->fs_relids = best_path->path.parent->relids;
/*
+ * If we're scanning a join relation, generate a recheck plan for
+ * EvalPlanQual support. (Irrelevant if scanning a base relation.)
+ */
+ if (scan_relid == 0)
+ {
+ scan_plan->fdw_recheck_plan =
+ create_plan_recurse(root, best_path->fdw_recheck_path);
+ scan_plan->fdw_recheck_plan->targetlist = scan_plan->fdw_scan_tlist;
+ }
+
+ /*
* Replace any outer-relation variables with nestloop params in the qual
* and fdw_exprs expressions. We do this last so that the FDW doesn't
* have to be involved. (Note that parts of fdw_exprs could have come
***************
*** 3758,3763 **** make_foreignscan(List *qptlist,
--- 3769,3776 ----
node->fdw_exprs = fdw_exprs;
node->fdw_private = fdw_private;
node->fdw_scan_tlist = fdw_scan_tlist;
+ /* fdw_recheck_plan will be filled in by create_foreignscan_plan */
+ node->fdw_recheck_plan = NULL;
node->fdw_recheck_quals = fdw_recheck_quals;
/* fs_relids will be filled in by create_foreignscan_plan */
node->fs_relids = NULL;
*** a/src/backend/optimizer/plan/setrefs.c
--- b/src/backend/optimizer/plan/setrefs.c
***************
*** 1130,1135 **** set_foreignscan_references(PlannerInfo *root,
--- 1130,1137 ----
/* fdw_scan_tlist itself just needs fix_scan_list() adjustments */
fscan->fdw_scan_tlist =
fix_scan_list(root, fscan->fdw_scan_tlist, rtoffset);
+ /* fdw_recheck_plan needs set_plan_refs() adjustments */
+ set_plan_refs(root, fscan->fdw_recheck_plan, rtoffset);
}
else
{
*** a/src/backend/optimizer/plan/subselect.c
--- b/src/backend/optimizer/plan/subselect.c
***************
*** 2405,2410 **** finalize_plan(PlannerInfo *root, Plan *plan, Bitmapset *valid_params,
--- 2405,2419 ----
/* We assume fdw_scan_tlist cannot contain Params */
context.paramids = bms_add_members(context.paramids,
scan_params);
+
+ /* recheck plan if foreign join */
+ if (fscan->scan.scanrelid == 0)
+ context.paramids =
+ bms_add_members(context.paramids,
+ finalize_plan(root,
+ fscan->fdw_recheck_plan,
+ valid_params,
+ scan_params));
}
break;
*** a/src/backend/optimizer/util/pathnode.c
--- b/src/backend/optimizer/util/pathnode.c
***************
*** 1488,1493 **** create_foreignscan_path(PlannerInfo *root, RelOptInfo *rel,
--- 1488,1494 ----
double rows, Cost startup_cost, Cost total_cost,
List *pathkeys,
Relids required_outer,
+ Path *fdw_recheck_path,
List *fdw_private)
{
ForeignPath *pathnode = makeNode(ForeignPath);
***************
*** 1501,1506 **** create_foreignscan_path(PlannerInfo *root, RelOptInfo *rel,
--- 1502,1508 ----
pathnode->path.total_cost = total_cost;
pathnode->path.pathkeys = pathkeys;
+ pathnode->fdw_recheck_path = fdw_recheck_path;
pathnode->fdw_private = fdw_private;
return pathnode;
*** a/src/include/nodes/execnodes.h
--- b/src/include/nodes/execnodes.h
***************
*** 1579,1584 **** typedef struct WorkTableScanState
--- 1579,1585 ----
typedef struct ForeignScanState
{
ScanState ss; /* its first field is NodeTag */
+ PlanState *fdw_recheck_plan; /* local join execution plan */
List *fdw_recheck_quals; /* original quals not in ss.ps.qual */
/* use struct pointer to avoid including fdwapi.h here */
struct FdwRoutine *fdwroutine;
*** a/src/include/nodes/plannodes.h
--- b/src/include/nodes/plannodes.h
***************
*** 529,534 **** typedef struct ForeignScan
--- 529,535 ----
List *fdw_exprs; /* expressions that FDW may evaluate */
List *fdw_private; /* private data for FDW */
List *fdw_scan_tlist; /* optional tlist describing scan tuple */
+ Plan *fdw_recheck_plan; /* local join execution plan */
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 */
*** a/src/include/nodes/relation.h
--- b/src/include/nodes/relation.h
***************
*** 903,912 **** typedef struct TidPath
--- 903,917 ----
* generally a good idea to use a representation that can be dumped by
* nodeToString(), so that you can examine the structure during debugging
* with tools like pprint().
+ *
+ * If a ForeignPath node represents a remote join, then fdw_recheck_path is
+ * a local join execution path for use in EvalPlanQual. (Else it is NULL.)
+ * The parameterization of fdw_recheck_path must be the same as that of path.
*/
typedef struct ForeignPath
{
Path path;
+ Path *fdw_recheck_path;
List *fdw_private;
} ForeignPath;
*** a/src/include/optimizer/pathnode.h
--- b/src/include/optimizer/pathnode.h
***************
*** 86,91 **** extern ForeignPath *create_foreignscan_path(PlannerInfo *root, RelOptInfo *rel,
--- 86,92 ----
double rows, Cost startup_cost, Cost total_cost,
List *pathkeys,
Relids required_outer,
+ Path *fdw_recheck_path,
List *fdw_private);
extern Relids calc_nestloop_required_outer(Path *outer_path, Path *inner_path);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
