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.

diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index e2f8078b23588262717f6af8e0719d8396052f37..13b04a06b1756375d1c55c13d0cf4efd0c1c0163 100644 (file)
--- 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 ddcd37ae286dca79646750d3c5c2a90fb8c2ec35..c2cb4b68835118f17700e96c7ca910b05eb8a1a4 100644 (file)
--- 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 6eb2bcc364db7db1e75524da5ef1f4a5a8773941..fb279e8b68e8453eefb387ce1243afcf5295e870 100644 (file)
--- 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 f5a284a26bffd5c86d10a29f62e99a815881cf03..fab526f399c86646db6db8d6389edc8d21ea4b97 100644 (file)
--- 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);
 }