Allow snapshot references to still work during transaction abort.

tglsfdc · tglsfdc · commit 57eb00909268 · 2011-09-26T22:25:28.000-04:00
In REPEATABLE READ (nee SERIALIZABLE) mode, an attempt to do
GetTransactionSnapshot() between AbortTransaction and CleanupTransaction
failed, because GetTransactionSnapshot would recompute the transaction
snapshot (which is already wrong, given the isolation mode) and then
re-register it in the TopTransactionResourceOwner, leading to an Assert
because the TopTransactionResourceOwner should be empty of resources after
AbortTransaction.  This is the root cause of bug #6218 from Yamamoto
Takashi.  While changing plancache.c to avoid requesting a snapshot when
handling a ROLLBACK masks the problem, I think this is really a snapmgr.c
bug: it's lower-level than the resource manager mechanism and should not be
shutting itself down before we unwind resource manager resources.  However,
just postponing the release of the transaction snapshot until cleanup time
didn't work because of the circular dependency with
TopTransactionResourceOwner.  Fix by managing the internal reference to
that snapshot manually instead of depending on TopTransactionResourceOwner.
This saves a few cycles as well as making the module layering more
straightforward.  predicate.c's dependencies on TopTransactionResourceOwner
go away too.

I think this is a longstanding bug, but there's no evidence that it's more
than a latent bug, so it doesn't seem worth any risk of back-patching.
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
@@ -1906,9 +1906,6 @@ CommitTransaction(void)
 	/* Clean up the relation cache */
 	AtEOXact_RelationCache(true);
 
-	/* Clean up the snapshot manager */
-	AtEarlyCommit_Snapshot();
-
 	/*
 	 * Make catalog changes visible to all backends.  This has to happen after
 	 * relcache references are dropped (see comments for
@@ -2158,9 +2155,6 @@ PrepareTransaction(void)
 	/* Clean up the relation cache */
 	AtEOXact_RelationCache(true);
 
-	/* Clean up the snapshot manager */
-	AtEarlyCommit_Snapshot();
-
 	/* notify doesn't need a postprepare call */
 
 	PostPrepare_PgStat();
@@ -2339,7 +2333,6 @@ AbortTransaction(void)
 		AtEOXact_ComboCid();
 		AtEOXact_HashTables(false);
 		AtEOXact_PgStat(false);
-		AtEOXact_Snapshot(false);
 		pgstat_report_xact_timestamp(0);
 	}
 
@@ -2368,6 +2361,7 @@ CleanupTransaction(void)
 	 * do abort cleanup processing
 	 */
 	AtCleanup_Portals();		/* now safe to release portal memory */
+	AtEOXact_Snapshot(false);	/* and release the transaction's snapshots */
 
 	CurrentResourceOwner = NULL;	/* and resource owner */
 	if (TopTransactionResourceOwner)
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
@@ -146,7 +146,7 @@
  *		PageIsPredicateLocked(Relation relation, BlockNumber blkno)
  *
  * predicate lock maintenance
- *		RegisterSerializableTransaction(Snapshot snapshot)
+ *		GetSerializableTransactionSnapshot(Snapshot snapshot)
  *		RegisterPredicateLockingXid(void)
  *		PredicateLockRelation(Relation relation, Snapshot snapshot)
  *		PredicateLockPage(Relation relation, BlockNumber blkno,
@@ -417,7 +417,7 @@ static void OldSerXidSetActiveSerXmin(TransactionId xid);
 static uint32 predicatelock_hash(const void *key, Size keysize);
 static void SummarizeOldestCommittedSxact(void);
 static Snapshot GetSafeSnapshot(Snapshot snapshot);
-static Snapshot RegisterSerializableTransactionInt(Snapshot snapshot);
+static Snapshot GetSerializableTransactionSnapshotInt(Snapshot snapshot);
 static bool PredicateLockExists(const PREDICATELOCKTARGETTAG *targettag);
 static bool GetParentPredicateLockTag(const PREDICATELOCKTARGETTAG *tag,
 						  PREDICATELOCKTARGETTAG *parent);
@@ -1485,6 +1485,10 @@ SummarizeOldestCommittedSxact(void)
  *		without further checks. This requires waiting for concurrent
  *		transactions to complete, and retrying with a new snapshot if
  *		one of them could possibly create a conflict.
+ *
+ *		As with GetSerializableTransactionSnapshot (which this is a subroutine
+ *		for), the passed-in Snapshot pointer should reference a static data
+ *		area that can safely be passed to GetSnapshotData.
  */
 static Snapshot
 GetSafeSnapshot(Snapshot origSnapshot)
@@ -1496,12 +1500,12 @@ GetSafeSnapshot(Snapshot origSnapshot)
 	while (true)
 	{
 		/*
-		 * RegisterSerializableTransactionInt is going to call
-		 * GetSnapshotData, so we need to provide it the static snapshot our
-		 * caller passed to us. It returns a copy of that snapshot and
-		 * registers it on TopTransactionResourceOwner.
+		 * GetSerializableTransactionSnapshotInt is going to call
+		 * GetSnapshotData, so we need to provide it the static snapshot area
+		 * our caller passed to us.  The pointer returned is actually the same
+		 * one passed to it, but we avoid assuming that here.
 		 */
-		snapshot = RegisterSerializableTransactionInt(origSnapshot);
+		snapshot = GetSerializableTransactionSnapshotInt(origSnapshot);
 
 		if (MySerializableXact == InvalidSerializableXact)
 			return snapshot;	/* no concurrent r/w xacts; it's safe */
@@ -1535,8 +1539,6 @@ GetSafeSnapshot(Snapshot origSnapshot)
 				(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
 				 errmsg("deferrable snapshot was unsafe; trying a new one")));
 		ReleasePredicateLocks(false);
-		UnregisterSnapshotFromOwner(snapshot,
-									TopTransactionResourceOwner);
 	}
 
 	/*
@@ -1549,28 +1551,34 @@ GetSafeSnapshot(Snapshot origSnapshot)
 }
 
 /*
- * Acquire and register a snapshot which can be used for this transaction..
+ * Acquire a snapshot that can be used for the current transaction.
+ *
  * Make sure we have a SERIALIZABLEXACT reference in MySerializableXact.
  * It should be current for this process and be contained in PredXact.
+ *
+ * The passed-in Snapshot pointer should reference a static data area that
+ * can safely be passed to GetSnapshotData.  The return value is actually
+ * always this same pointer; no new snapshot data structure is allocated
+ * within this function.
  */
 Snapshot
-RegisterSerializableTransaction(Snapshot snapshot)
+GetSerializableTransactionSnapshot(Snapshot snapshot)
 {
 	Assert(IsolationIsSerializable());
 
 	/*
 	 * A special optimization is available for SERIALIZABLE READ ONLY
 	 * DEFERRABLE transactions -- we can wait for a suitable snapshot and
-	 * thereby avoid all SSI overhead once it's running..
+	 * thereby avoid all SSI overhead once it's running.
 	 */
 	if (XactReadOnly && XactDeferrable)
 		return GetSafeSnapshot(snapshot);
 
-	return RegisterSerializableTransactionInt(snapshot);
+	return GetSerializableTransactionSnapshotInt(snapshot);
 }
 
 static Snapshot
-RegisterSerializableTransactionInt(Snapshot snapshot)
+GetSerializableTransactionSnapshotInt(Snapshot snapshot)
 {
 	PGPROC	   *proc;
 	VirtualTransactionId vxid;
@@ -1607,9 +1615,8 @@ RegisterSerializableTransactionInt(Snapshot snapshot)
 		}
 	} while (!sxact);
 
-	/* Get and register a snapshot */
+	/* Get the snapshot */
 	snapshot = GetSnapshotData(snapshot);
-	snapshot = RegisterSnapshotOnOwner(snapshot, TopTransactionResourceOwner);
 
 	/*
 	 * If there are no serializable transactions which are not read-only, we
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
@@ -7,6 +7,14 @@
  * persistent memory.  When a snapshot is no longer in any of these lists
  * (tracked by separate refcounts on each snapshot), its memory can be freed.
  *
+ * The FirstXactSnapshot, if any, is treated a bit specially: we increment its
+ * regd_count and count it in RegisteredSnapshots, but this reference is not
+ * tracked by a resource owner. We used to use the TopTransactionResourceOwner
+ * to track this snapshot reference, but that introduces logical circularity
+ * and thus makes it impossible to clean up in a sane fashion.  It's better to
+ * handle this reference as an internally-tracked registration, so that this
+ * module is entirely lower-level than ResourceOwners.
+ *
  * These arrangements let us reset MyProc->xmin when there are no snapshots
  * referenced by this transaction.	(One possible improvement would be to be
  * able to advance Xmin when the snapshot with the earliest Xmin is no longer
@@ -97,11 +105,11 @@ static int	RegisteredSnapshots = 0;
 bool		FirstSnapshotSet = false;
 
 /*
- * Remembers whether this transaction registered a transaction snapshot at
- * start.  We cannot trust FirstSnapshotSet in combination with
- * IsolationUsesXactSnapshot(), because GUC may be reset before us.
+ * Remember the serializable transaction snapshot, if any.  We cannot trust
+ * FirstSnapshotSet in combination with IsolationUsesXactSnapshot(), because
+ * GUC may be reset before us, changing the value of IsolationUsesXactSnapshot.
  */
-static bool registered_xact_snapshot = false;
+static Snapshot FirstXactSnapshot = NULL;
 
 
 static Snapshot CopySnapshot(Snapshot snapshot);
@@ -125,23 +133,27 @@ GetTransactionSnapshot(void)
 	if (!FirstSnapshotSet)
 	{
 		Assert(RegisteredSnapshots == 0);
+		Assert(FirstXactSnapshot == NULL);
 
 		/*
 		 * In transaction-snapshot mode, the first snapshot must live until
 		 * end of xact regardless of what the caller does with it, so we must
-		 * register it internally here and unregister it at end of xact.
+		 * make a copy of it rather than returning CurrentSnapshotData
+		 * directly.
 		 */
 		if (IsolationUsesXactSnapshot())
 		{
+			/* First, create the snapshot in CurrentSnapshotData */
 			if (IsolationIsSerializable())
-				CurrentSnapshot = RegisterSerializableTransaction(&CurrentSnapshotData);
+				CurrentSnapshot = GetSerializableTransactionSnapshot(&CurrentSnapshotData);
 			else
-			{
 				CurrentSnapshot = GetSnapshotData(&CurrentSnapshotData);
-				CurrentSnapshot = RegisterSnapshotOnOwner(CurrentSnapshot,
-												TopTransactionResourceOwner);
-			}
-			registered_xact_snapshot = true;
+			/* Make a saved copy */
+			CurrentSnapshot = CopySnapshot(CurrentSnapshot);
+			FirstXactSnapshot = CurrentSnapshot;
+			/* Mark it as "registered" in FirstXactSnapshot */
+			FirstXactSnapshot->regd_count++;
+			RegisteredSnapshots++;
 		}
 		else
 			CurrentSnapshot = GetSnapshotData(&CurrentSnapshotData);
@@ -522,33 +534,30 @@ AtSubAbort_Snapshot(int level)
 	SnapshotResetXmin();
 }
 
-/*
- * AtEarlyCommit_Snapshot
- *
- * Snapshot manager's cleanup function, to be called on commit, before
- * doing resowner.c resource release.
- */
-void
-AtEarlyCommit_Snapshot(void)
-{
-	/*
-	 * In transaction-snapshot mode we must unregister our private refcount to
-	 * the transaction-snapshot.
-	 */
-	if (registered_xact_snapshot)
-		UnregisterSnapshotFromOwner(CurrentSnapshot,
-									TopTransactionResourceOwner);
-	registered_xact_snapshot = false;
-
-}
-
 /*
  * AtEOXact_Snapshot
  *		Snapshot manager's cleanup function for end of transaction
  */
 void
 AtEOXact_Snapshot(bool isCommit)
 {
+	/*
+	 * In transaction-snapshot mode we must release our privately-managed
+	 * reference to the transaction snapshot.  We must decrement
+	 * RegisteredSnapshots to keep the check below happy.  But we don't bother
+	 * to do FreeSnapshot, for two reasons: the memory will go away with
+	 * TopTransactionContext anyway, and if someone has left the snapshot
+	 * stacked as active, we don't want the code below to be chasing through
+	 * a dangling pointer.
+	 */
+	if (FirstXactSnapshot != NULL)
+	{
+		Assert(FirstXactSnapshot->regd_count > 0);
+		Assert(RegisteredSnapshots > 0);
+		RegisteredSnapshots--;
+	}
+	FirstXactSnapshot = NULL;
+
 	/* On commit, complain about leftover snapshots */
 	if (isCommit)
 	{
@@ -574,5 +583,6 @@ AtEOXact_Snapshot(bool isCommit)
 	SecondarySnapshot = NULL;
 
 	FirstSnapshotSet = false;
-	registered_xact_snapshot = false;
+
+	SnapshotResetXmin();
 }
diff --git a/src/include/storage/predicate.h b/src/include/storage/predicate.h
@@ -42,7 +42,7 @@ extern void CheckPointPredicate(void);
 extern bool PageIsPredicateLocked(Relation relation, BlockNumber blkno);
 
 /* predicate lock maintenance */
-extern Snapshot RegisterSerializableTransaction(Snapshot snapshot);
+extern Snapshot GetSerializableTransactionSnapshot(Snapshot snapshot);
 extern void RegisterPredicateLockingXid(TransactionId xid);
 extern void PredicateLockRelation(Relation relation, Snapshot snapshot);
 extern void PredicateLockPage(Relation relation, BlockNumber blkno, Snapshot snapshot);
diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h
@@ -40,7 +40,6 @@ extern void UnregisterSnapshotFromOwner(Snapshot snapshot, ResourceOwner owner);
 
 extern void AtSubCommit_Snapshot(int level);
 extern void AtSubAbort_Snapshot(int level);
-extern void AtEarlyCommit_Snapshot(void);
 extern void AtEOXact_Snapshot(bool isCommit);
 
 #endif   /* SNAPMGR_H */
