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.

Author: tglsfdc

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

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)

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 */

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);
 }