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 f451fd70551d8605b56c8cf4c6970f1085c7e894..a2b0c345456875293d36ecc5b6043b4d881d3242 100644 (file)
--- 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 c9423be64f571eb5bf5096764430ecc36a2a05cb..d5b6bc31d894222ad56dd2357433f295ec9b85c3 100644 (file)
--- 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 8752597976901241a208a19ba1df4ce1dad15f5b..497b31309c2d9aea9344da477cdfb407ab5aa062 100644 (file)
--- 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 449fd1d30057857c187f276aa3e84cc64956b558..8ff2f4227d2684c74d62113953cd16d06403045f 100644 (file)
--- 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);
 }