From 43f2f4a31d57d5ea9c4f368285773fffce8fbc5c Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Wed, 13 Nov 2019 14:19:40 -0500
Subject: [PATCH v3] Move AtEOXact_Snapshot() back to AbortTransaction().

As a general rule, it seems preferable to do as much work as possible
in AbortTransaction(), leaving as little work as possible to be done
in CleanupTransaction(), because it's a good idea to hold on to
resources for the shortest possible amount of time.

Way back in 2011, commit 57eb009092684e6e1788dd0dae641ccee1668b10
moved AbortTransaction's AtEOXact_Snapshot call to
CleanupTransaction to fix a problem when a ROLLBACK statement was
prepared at the protocol level and executed in a transaction
with REPEATABLE READ or higher isolation. RevalidateCachedQuery
would attempt to obtain a snapshot and fail because the transaction
was already gone.  Commit ac63dca607e8e22247defbc8fe03b6baa3628c42
subsequently taught RevalidateCachedQuery not to obtain a snapshot
for such commands after all while fixing an unrelated bug, and
there now seems to be no case in which we obtain a snapshot in
an aborted transaction.

That's good, because it can't possibly be safe to use a snapshot in
an aborted transaction, for reasons explained in the comments
added by this patch. So, move the call back to AbortTransaction().
To help catch future cases of unsafe snapshot use, add a bunch of
Assert() statements to snapmgr.c to flag dangerous coding patterns.
Flesh out the comments in RevalidateCachedQuery a bit, too.

Patch by me, reviewed by Andres Freund.

Discussion: http://postgr.es/m/CA+TgmoZKo449=6cTzRkVvaWVpOJT__Cwy=wFtW32bB31sNRLfg@mail.gmail.com
---
 src/backend/access/transam/xact.c   | 19 +++++++++++++++----
 src/backend/storage/ipc/procarray.c |  3 +++
 src/backend/utils/cache/plancache.c |  6 ++++++
 src/backend/utils/time/snapmgr.c    | 27 +++++++++++++++++++++++++++
 4 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 8fe38c33c6..f718b47540 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2695,8 +2695,8 @@ AbortTransaction(void)
 
 	/*
 	 * Post-abort cleanup.  See notes in CommitTransaction() concerning
-	 * ordering.  We can skip all of it if the transaction failed before
-	 * creating a resource owner.
+	 * ordering.  We can skip quite a bit of work if the transaction failed
+	 * before creating a resource owner.
 	 */
 	if (TopTransactionResourceOwner != NULL)
 	{
@@ -2734,6 +2734,18 @@ AbortTransaction(void)
 		pgstat_report_xact_timestamp(0);
 	}
 
+	/*
+	 * Any snapshots taken by this transaction were unsafe to use even at the
+	 * time when we entered AbortTransaction(), since we might have, for
+	 * example, inserted a heap tuple and failed while inserting index tuples.
+	 * They were even more unsafe after ProcArrayEndTransaction(), since after
+	 * that point tuples we inserted could be pruned by other backends.
+	 * However, we postpone the cleanup until this point in the sequence
+	 * because it has to be done after ResourceOwnerRelease() has finishing
+	 * unregistering snapshots.
+	 */
+	AtEOXact_Snapshot(false, true);
+
 	/*
 	 * State remains TRANS_ABORT until CleanupTransaction().
 	 */
@@ -2759,7 +2771,6 @@ CleanupTransaction(void)
 	 * do abort cleanup processing
 	 */
 	AtCleanup_Portals();		/* now safe to release portal memory */
-	AtEOXact_Snapshot(false, true); /* and release the transaction's snapshots */
 
 	CurrentResourceOwner = NULL;	/* and resource owner */
 	if (TopTransactionResourceOwner)
@@ -4954,7 +4965,7 @@ AbortSubTransaction(void)
 	}
 
 	/*
-	 * We can skip all this stuff if the subxact failed before creating a
+	 * We can skip quite a bit of work if the subxact failed before creating a
 	 * ResourceOwner...
 	 */
 	if (s->curTransactionOwner)
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 13bcbe77de..8374422137 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -1517,6 +1517,9 @@ GetSnapshotData(Snapshot snapshot)
 
 	Assert(snapshot != NULL);
 
+	/* Unsafe if transaction is aborted, or if we're not in a transaction. */
+	Assert(IsTransactionState());
+
 	/*
 	 * Allocating space for maxProcs xids is usually overkill; numProcs would
 	 * be sufficient.  But it seems better to do the malloc while not holding
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index abc3062892..732c421690 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -663,6 +663,12 @@ RevalidateCachedQuery(CachedPlanSource *plansource,
 	 * checking whether parse analysis requires a snapshot; utility commands
 	 * don't have invalidatable plans, so we'd not get here for such a
 	 * command.
+	 *
+	 * Note that this code shouldn't be reached from within a failed
+	 * transaction, because only transaction control commands are legal in
+	 * that context, and we've already checked for those. That's important,
+	 * because we can't safely call GetTransactionSnapshot() from a failed
+	 * transaction.
 	 */
 	snapshot_set = false;
 	if (!ActiveSnapshotSet())
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 47b0517596..109af7f997 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -305,6 +305,9 @@ SnapMgrInit(void)
 Snapshot
 GetTransactionSnapshot(void)
 {
+	/* Unsafe if transaction is aborted, or if we're not in a transaction. */
+	Assert(IsTransactionState());
+
 	/*
 	 * Return historic snapshot if doing logical decoding. We'll never need a
 	 * non-historic transaction snapshot in this (sub-)transaction, so there's
@@ -380,6 +383,9 @@ GetTransactionSnapshot(void)
 Snapshot
 GetLatestSnapshot(void)
 {
+	/* Unsafe if transaction is aborted, or if we're not in a transaction. */
+	Assert(IsTransactionState());
+
 	/*
 	 * We might be able to relax this, but nothing that could otherwise work
 	 * needs it.
@@ -415,6 +421,9 @@ GetOldestSnapshot(void)
 	Snapshot	OldestRegisteredSnapshot = NULL;
 	XLogRecPtr	RegisteredLSN = InvalidXLogRecPtr;
 
+	/* Unsafe if transaction is aborted, or if we're not in a transaction. */
+	Assert(IsTransactionState());
+
 	if (!pairingheap_is_empty(&RegisteredSnapshots))
 	{
 		OldestRegisteredSnapshot = pairingheap_container(SnapshotData, ph_node,
@@ -441,6 +450,9 @@ GetOldestSnapshot(void)
 Snapshot
 GetCatalogSnapshot(Oid relid)
 {
+	/* Unsafe if transaction is aborted, or if we're not in a transaction. */
+	Assert(IsTransactionState());
+
 	/*
 	 * Return historic snapshot while we're doing logical decoding, so we can
 	 * see the appropriate state of the catalog.
@@ -463,6 +475,9 @@ GetCatalogSnapshot(Oid relid)
 Snapshot
 GetNonHistoricCatalogSnapshot(Oid relid)
 {
+	/* Unsafe if transaction is aborted, or if we're not in a transaction. */
+	Assert(IsTransactionState());
+
 	/*
 	 * If the caller is trying to scan a relation that has no syscache, no
 	 * catcache invalidations will be sent when it is updated.  For a few key
@@ -736,6 +751,9 @@ PushActiveSnapshot(Snapshot snap)
 {
 	ActiveSnapshotElt *newactive;
 
+	/* Unsafe if transaction is aborted, or if we're not in a transaction. */
+	Assert(IsTransactionState());
+
 	Assert(snap != InvalidSnapshot);
 
 	newactive = MemoryContextAlloc(TopTransactionContext, sizeof(ActiveSnapshotElt));
@@ -789,6 +807,9 @@ UpdateActiveSnapshotCommandId(void)
 	Assert(ActiveSnapshot->as_snap->active_count == 1);
 	Assert(ActiveSnapshot->as_snap->regd_count == 0);
 
+	/* This is not sensible except within a valid transaction. */
+	Assert(IsTransactionState());
+
 	/*
 	 * Don't allow modification of the active snapshot during parallel
 	 * operation.  We share the snapshot to worker backends at the beginning
@@ -842,6 +863,9 @@ GetActiveSnapshot(void)
 {
 	Assert(ActiveSnapshot != NULL);
 
+	/* Unsafe if transaction is aborted, or if we're not in a transaction. */
+	Assert(IsTransactionState());
+
 	return ActiveSnapshot->as_snap;
 }
 
@@ -882,6 +906,9 @@ RegisterSnapshotOnOwner(Snapshot snapshot, ResourceOwner owner)
 	if (snapshot == InvalidSnapshot)
 		return InvalidSnapshot;
 
+	/* It's unsafe to use snapshots in an aborted transaction. */
+	Assert(!IsAbortedTransactionBlockState());
+
 	/* Static snapshot?  Create a persistent copy */
 	snap = snapshot->copied ? snapshot : CopySnapshot(snapshot);
 
-- 
2.17.2 (Apple Git-113)

