From 51048ab79896da2f3c6a2e47f6fa1fdd734539b8 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 15 May 2005 21:19:55 +0000 Subject: [PATCH] Fix latent bug in ExecSeqRestrPos: it leaves the plan node's result slot in an inconsistent state. (This is only latent because in reality ExecSeqRestrPos is dead code at the moment ... but someday maybe it won't be.) Add some comments about what the API for plan node mark/restore actually is, because it's not immediately obvious. --- src/backend/access/index/indexam.c | 4 ++++ src/backend/executor/execAmi.c | 13 +++++++++++++ src/backend/executor/nodeMergejoin.c | 6 ++++-- src/backend/executor/nodeSeqscan.c | 14 ++++++++++---- 4 files changed, 31 insertions(+), 6 deletions(-) diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c index f451fd7055..a2b0c34545 100644 --- a/src/backend/access/index/indexam.c +++ b/src/backend/access/index/indexam.c @@ -411,6 +411,10 @@ index_markpos(IndexScanDesc scan) /* ---------------- * index_restrpos - restore a scan position + * + * NOTE: this only restores the internal scan state of the index AM. + * The current result tuple (scan->xs_ctup) doesn't change. See comments + * for ExecRestrPos(). * ---------------- */ void diff --git a/src/backend/executor/execAmi.c b/src/backend/executor/execAmi.c index c9423be64f..d5b6bc31d8 100644 --- a/src/backend/executor/execAmi.c +++ b/src/backend/executor/execAmi.c @@ -248,6 +248,14 @@ ExecMarkPos(PlanState *node) * ExecRestrPos * * restores the scan position previously saved with ExecMarkPos() + * + * NOTE: the semantics of this are that the first ExecProcNode following + * the restore operation will yield the same tuple as the first one following + * the mark operation. It is unspecified what happens to the plan node's + * result TupleTableSlot. (In most cases the result slot is unchanged by + * a restore, but the node may choose to clear it or to load it with the + * restored-to tuple.) Hence the caller should discard any previously + * returned TupleTableSlot after doing a restore. */ void ExecRestrPos(PlanState *node) @@ -290,6 +298,11 @@ ExecRestrPos(PlanState *node) * XXX Ideally, all plan node types would support mark/restore, and this * wouldn't be needed. For now, this had better match the routines above. * But note the test is on Plan nodetype, not PlanState nodetype. + * + * (However, since the only present use of mark/restore is in mergejoin, + * there is no need to support mark/restore in any plan type that is not + * capable of generating ordered output. So the seqscan, tidscan, and + * functionscan support is actually useless code at present.) */ bool ExecSupportsMarkRestore(NodeTag plantype) diff --git a/src/backend/executor/nodeMergejoin.c b/src/backend/executor/nodeMergejoin.c index 8752597976..497b31309c 100644 --- a/src/backend/executor/nodeMergejoin.c +++ b/src/backend/executor/nodeMergejoin.c @@ -1134,8 +1134,10 @@ ExecMergeJoin(MergeJoinState *node) ExecRestrPos(innerPlan); /* - * ExecRestrPos really should give us back a new Slot, - * but since it doesn't, use the marked slot. + * ExecRestrPos probably should give us back a new Slot, + * but since it doesn't, use the marked slot. (The + * previously returned mj_InnerTupleSlot cannot be + * assumed to hold the required tuple.) */ node->mj_InnerTupleSlot = innerTupleSlot; /* we need not do MJEvalInnerValues again */ diff --git a/src/backend/executor/nodeSeqscan.c b/src/backend/executor/nodeSeqscan.c index 449fd1d300..8ff2f4227d 100644 --- a/src/backend/executor/nodeSeqscan.c +++ b/src/backend/executor/nodeSeqscan.c @@ -342,9 +342,8 @@ ExecSeqReScan(SeqScanState *node, ExprContext *exprCtxt) void ExecSeqMarkPos(SeqScanState *node) { - HeapScanDesc scan; + HeapScanDesc scan = node->ss_currentScanDesc; - scan = node->ss_currentScanDesc; heap_markpos(scan); } @@ -357,8 +356,15 @@ ExecSeqMarkPos(SeqScanState *node) void ExecSeqRestrPos(SeqScanState *node) { - HeapScanDesc scan; + HeapScanDesc scan = node->ss_currentScanDesc; + + /* + * Clear any reference to the previously returned tuple. This is + * needed because the slot is simply pointing at scan->rs_cbuf, which + * heap_restrpos will change; we'd have an internally inconsistent + * slot if we didn't do this. + */ + ExecClearTuple(node->ss_ScanTupleSlot); - scan = node->ss_currentScanDesc; heap_restrpos(scan); } -- 2.39.5