Fix snapshot management, take two.

Partially revert the previous patch I installed and replace it with a more
general fix: any time a snapshot is pushed as Active, we need to ensure that it
will not be modified in the future.  This means that if the same snapshot is
used as CurrentSnapshot, it needs to be copied separately.  This affects
serializable transactions only, because CurrentSnapshot has already been copied
by RegisterSnapshot and so PushActiveSnapshot does not think it needs another
copy.  However, CommandCounterIncrement would modify CurrentSnapshot, whereas
ActiveSnapshots must not have their command counters incremented.

I say "partially" because the regression test I added for the previous bug
has been kept.

(This restores 8.3 behavior, because before snapmgr.c existed, any snapshot set
as Active was copied.)

Per bug report from Stuart Bishop in
6bc73d4c0910042358k3d1adff3qa36f8df75198ecea@mail.gmail.com

1 files changed, 16 insertions, 6 deletions

diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 12ca81616d1..9c06124ba95 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -19,7 +19,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/utils/time/snapmgr.c,v 1.10.2.1 2009/10/02 17:58:21 alvherre Exp $
+ *	  $PostgreSQL: pgsql/src/backend/utils/time/snapmgr.c,v 1.10.2.2 2009/10/07 16:27:28 alvherre Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -104,6 +104,7 @@ bool		FirstSnapshotSet = false;
 static bool registered_serializable = false;
 
 
+static Snapshot CopySnapshot(Snapshot snapshot);
 static void FreeSnapshot(Snapshot snapshot);
 static void SnapshotResetXmin(void);
 
@@ -191,7 +192,7 @@ SnapshotSetCommandId(CommandId curcid)
  * The copy is palloc'd in TopTransactionContext and has initial refcounts set
  * to 0.  The returned snapshot has the copied flag set.
  */
-Snapshot
+static Snapshot
 CopySnapshot(Snapshot snapshot)
 {
 	Snapshot	newsnap;
@@ -254,8 +255,9 @@ FreeSnapshot(Snapshot snapshot)
  * PushActiveSnapshot
  *		Set the given snapshot as the current active snapshot
  *
- * If this is the first use of this snapshot, create a new long-lived copy with
- * active refcount=1.  Otherwise, only increment the refcount.
+ * If the passed snapshot is a statically-allocated one, or it is possibly
+ * subject to a future command counter update, create a new long-lived copy
+ * with active refcount=1.  Otherwise, only increment the refcount.
  */
 void
 PushActiveSnapshot(Snapshot snap)
@@ -265,8 +267,16 @@ PushActiveSnapshot(Snapshot snap)
 	Assert(snap != InvalidSnapshot);
 
 	newactive = MemoryContextAlloc(TopTransactionContext, sizeof(ActiveSnapshotElt));
-	/* Static snapshot?  Create a persistent copy */
-	newactive->as_snap = snap->copied ? snap : CopySnapshot(snap);
+
+	/*
+	 * Checking SecondarySnapshot is probably useless here, but it seems better
+	 * to be sure.
+	 */
+	if (snap == CurrentSnapshot || snap == SecondarySnapshot || !snap->copied)
+		newactive->as_snap = CopySnapshot(snap);
+	else
+		newactive->as_snap = snap;
+
 	newactive->as_next = ActiveSnapshot;
 	newactive->as_level = GetCurrentTransactionNestLevel();