From 2ef172a2a4d8c093c0f2d4d580d5aab1ca8dcffe 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 | 6 +++++- src/backend/executor/execAmi.c | 15 ++++++++++++++- src/backend/executor/nodeMergejoin.c | 8 +++++--- src/backend/executor/nodeSeqscan.c | 16 +++++++++++----- 4 files changed, 35 insertions(+), 10 deletions(-) diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c index e2f8078b23..13b04a06b1 100644 --- a/src/backend/access/index/indexam.c +++ b/src/backend/access/index/indexam.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/access/index/indexam.c,v 1.80 2005/04/14 20:03:23 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/access/index/indexam.c,v 1.81 2005/05/15 21:19:54 tgl Exp $ * * INTERFACE ROUTINES * index_open - open an index relation by relation OID @@ -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 ddcd37ae28..c2cb4b6883 100644 --- a/src/backend/executor/execAmi.c +++ b/src/backend/executor/execAmi.c @@ -6,7 +6,7 @@ * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/backend/executor/execAmi.c,v 1.83 2005/04/19 22:35:11 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/executor/execAmi.c,v 1.84 2005/05/15 21:19:54 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -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 6eb2bcc364..fb279e8b68 100644 --- a/src/backend/executor/nodeMergejoin.c +++ b/src/backend/executor/nodeMergejoin.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/executor/nodeMergejoin.c,v 1.73 2005/05/14 21:29:23 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/executor/nodeMergejoin.c,v 1.74 2005/05/15 21:19:55 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -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 f5a284a26b..fab526f399 100644 --- a/src/backend/executor/nodeSeqscan.c +++ b/src/backend/executor/nodeSeqscan.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/executor/nodeSeqscan.c,v 1.52 2005/03/16 21:38:07 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/executor/nodeSeqscan.c,v 1.53 2005/05/15 21:19:55 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -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); }