Allow snapshot references to still work during transaction abort.

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.

1 files changed, 23 insertions, 16 deletions

diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 1754180453d..d39f8975f88 100644
--- 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