Improve documentation about reasoning behind the order of operations

in GetSnapshotData, GetNewTransactionId, CommitTransaction, AbortTransaction,
etc.  Correct race condition in transaction status testing in
HeapTupleSatisfiesVacuum --- this wasn't important for old VACUUM with
exclusive lock on its table, but it sure is important now.  All per
pghackers discussion 7/11/01 and 7/12/01.

2 files changed, 43 insertions, 25 deletions

diff --git a/src/backend/access/transam/varsup.c b/src/backend/access/transam/varsup.c
index 2b253fc5855..857f2c3d3e8 100644
--- a/src/backend/access/transam/varsup.c
+++ b/src/backend/access/transam/varsup.c
@@ -6,7 +6,7 @@
  * Copyright (c) 2000, PostgreSQL Global Development Group
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/access/transam/varsup.c,v 1.41 2001/07/12 04:11:13 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/access/transam/varsup.c,v 1.42 2001/07/16 22:43:33 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -60,8 +60,16 @@ GetNewTransactionId(TransactionId *xid)
 	 * XXX by storing xid into MyProc without acquiring SInvalLock, we are
 	 * relying on fetch/store of an xid to be atomic, else other backends
 	 * might see a partially-set xid here.  But holding both locks at once
-	 * would be a nasty concurrency hit (and at this writing, could cause a
-	 * deadlock against GetSnapshotData).  So for now, assume atomicity.
+	 * would be a nasty concurrency hit (and in fact could cause a deadlock
+	 * against GetSnapshotData).  So for now, assume atomicity.  Note that
+	 * readers of PROC xid field should be careful to fetch the value only
+	 * once, rather than assume they can read it multiple times and get the
+	 * same answer each time.
+	 *
+	 * A solution to the atomic-store problem would be to give each PROC
+	 * its own spinlock used only for fetching/storing that PROC's xid.
+	 * (SInvalLock would then mean primarily that PROCs couldn't be added/
+	 * removed while holding the lock.)
 	 */
 	if (MyProc != (PROC *) NULL)
 		MyProc->xid = *xid;
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index d32a6dda978..f35e8d9203e 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/access/transam/xact.c,v 1.107 2001/07/15 22:48:16 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/access/transam/xact.c,v 1.108 2001/07/16 22:43:33 tgl Exp $
  *
  * NOTES
  *		Transaction aborts can now occur two ways:
@@ -1018,16 +1018,21 @@ CommitTransaction(void)
 
 	CloseSequences();
 	AtEOXact_portals();
+
+	/* Here is where we really truly commit. */
 	RecordTransactionCommit();
 
 	/*
 	 * Let others know about no transaction in progress by me. Note that
-	 * this must be done _before_ releasing locks we hold and
+	 * this must be done _before_ releasing locks we hold and _after_
+	 * RecordTransactionCommit.
+	 *
 	 * SpinAcquire(SInvalLock) is required: UPDATE with xid 0 is blocked
 	 * by xid 1' UPDATE, xid 1 is doing commit while xid 2 gets snapshot -
 	 * if xid 2' GetSnapshotData sees xid 1 as running then it must see
 	 * xid 0 as running as well or it will see two tuple versions - one
-	 * deleted by xid 1 and one inserted by xid 0.
+	 * deleted by xid 1 and one inserted by xid 0.  See notes in
+	 * GetSnapshotData.
 	 */
 	if (MyProc != (PROC *) NULL)
 	{
@@ -1038,6 +1043,12 @@ CommitTransaction(void)
 		SpinRelease(SInvalLock);
 	}
 
+	/*
+	 * This is all post-commit cleanup.  Note that if an error is raised
+	 * here, it's too late to abort the transaction.  This should be just
+	 * noncritical resource releasing.
+	 */
+
 	RelationPurgeLocalRelation(true);
 	AtEOXact_temp_relations(true);
 	smgrDoPendingDeletes(true);
@@ -1081,23 +1092,6 @@ AbortTransaction(void)
 	HOLD_INTERRUPTS();
 
 	/*
-	 * Let others to know about no transaction in progress - vadim
-	 * 11/26/96
-	 *
-	 * XXX it'd be nice to acquire SInvalLock for this, but too much risk of
-	 * lockup: what if we were holding SInvalLock when we elog'd?  Net effect
-	 * is that we are relying on fetch/store of an xid to be atomic, else
-	 * other backends might see a partially-zeroed xid here.  Would it be
-	 * safe to release spins before we reset xid/xmin?  But see also 
-	 * GetNewTransactionId, which does the same thing.
-	 */
-	if (MyProc != (PROC *) NULL)
-	{
-		MyProc->xid = InvalidTransactionId;
-		MyProc->xmin = InvalidTransactionId;
-	}
-
-	/*
 	 * Release any spinlocks or buffer context locks we might be holding
 	 * as quickly as possible.	(Real locks, however, must be held till we
 	 * finish aborting.)  Releasing spinlocks is critical since we might
@@ -1143,10 +1137,23 @@ AbortTransaction(void)
 	AtAbort_Notify();
 	CloseSequences();
 	AtEOXact_portals();
+
+	/* Advertise the fact that we aborted in pg_log. */
 	RecordTransactionAbort();
 
-	/* Count transaction abort in statistics collector */
-	pgstat_count_xact_rollback();
+	/*
+	 * Let others know about no transaction in progress by me. Note that
+	 * this must be done _before_ releasing locks we hold and _after_
+	 * RecordTransactionAbort.
+	 */
+	if (MyProc != (PROC *) NULL)
+	{
+		/* Lock SInvalLock because that's what GetSnapshotData uses. */
+		SpinAcquire(SInvalLock);
+		MyProc->xid = InvalidTransactionId;
+		MyProc->xmin = InvalidTransactionId;
+		SpinRelease(SInvalLock);
+	}
 
 	RelationPurgeLocalRelation(false);
 	AtEOXact_temp_relations(false);
@@ -1165,6 +1172,9 @@ AbortTransaction(void)
 
 	SharedBufferChanged = false;/* safest place to do it */
 
+	/* Count transaction abort in statistics collector */
+	pgstat_count_xact_rollback();
+
 	/*
 	 * State remains TRANS_ABORT until CleanupTransaction().
 	 */