Replace the former method of determining snapshot xmax --- to wit, calling

ReadNewTransactionId from GetSnapshotData --- with a "latestCompletedXid"
variable that is updated during transaction commit or abort.  Since
latestCompletedXid is written only in places that had to lock ProcArrayLock
exclusively anyway, and is read only in places that had to lock ProcArrayLock
shared anyway, it adds no new locking requirements to the system despite being
cluster-wide.  Moreover, removing ReadNewTransactionId from snapshot
acquisition eliminates the need to take both XidGenLock and ProcArrayLock at
the same time.  Since XidGenLock is sometimes held across I/O this can be a
significant win.  Some preliminary benchmarking suggested that this patch has
no effect on average throughput but can significantly improve the worst-case
transaction times seen in pgbench.  Concept by Florian Pflug, implementation
by Tom Lane.

1 files changed, 32 insertions, 104 deletions

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 02b064179f2..1b16f7111b2 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -10,7 +10,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.249 2007/09/07 20:59:26 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.250 2007/09/08 20:31:14 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -233,7 +233,7 @@ static void CallSubXactCallbacks(SubXactEvent event,
 					 SubTransactionId parentSubid);
 static void CleanupTransaction(void);
 static void CommitTransaction(void);
-static void RecordTransactionAbort(bool isSubXact);
+static TransactionId RecordTransactionAbort(bool isSubXact);
 static void StartTransaction(void);
 
 static void RecordSubTransactionCommit(void);
@@ -748,13 +748,17 @@ AtSubStart_ResourceOwner(void)
 /*
  *	RecordTransactionCommit
  *
+ * Returns latest XID among xact and its children, or InvalidTransactionId
+ * if the xact has no XID.  (We compute that here just because it's easier.)
+ *
  * This is exported only to support an ugly hack in VACUUM FULL.
  */
-void
+TransactionId
 RecordTransactionCommit(void)
 {
 	TransactionId xid = GetTopTransactionIdIfAny();
 	bool 		markXidCommitted = TransactionIdIsValid(xid);
+	TransactionId latestXid = InvalidTransactionId;
 	int			nrels;
 	RelFileNode *rels;
 	bool		haveNonTemp;
@@ -930,6 +934,9 @@ RecordTransactionCommit(void)
 		END_CRIT_SECTION();
 	}
 
+	/* Compute latestXid while we have the child XIDs handy */
+	latestXid = TransactionIdLatest(xid, nchildren, children);
+
 	/* Reset XactLastRecEnd until the next transaction writes something */
 	XactLastRecEnd.xrecoff = 0;
 
@@ -939,6 +946,8 @@ cleanup:
 		pfree(rels);
 	if (children)
 		pfree(children);
+
+	return latestXid;
 }
 
 
@@ -1084,11 +1093,15 @@ RecordSubTransactionCommit(void)
 
 /*
  *	RecordTransactionAbort
+ *
+ * Returns latest XID among xact and its children, or InvalidTransactionId
+ * if the xact has no XID.  (We compute that here just because it's easier.)
  */
-static void
+static TransactionId
 RecordTransactionAbort(bool isSubXact)
 {
 	TransactionId xid = GetCurrentTransactionIdIfAny();
+	TransactionId latestXid;
 	int			nrels;
 	RelFileNode *rels;
 	int			nchildren;
@@ -1108,7 +1121,7 @@ RecordTransactionAbort(bool isSubXact)
 		/* Reset XactLastRecEnd until the next transaction writes something */
 		if (!isSubXact)
 			XactLastRecEnd.xrecoff = 0;
-		return;
+		return InvalidTransactionId;
 	}
 
 	/*
@@ -1186,6 +1199,9 @@ RecordTransactionAbort(bool isSubXact)
 
 	END_CRIT_SECTION();
 
+	/* Compute latestXid while we have the child XIDs handy */
+	latestXid = TransactionIdLatest(xid, nchildren, children);
+
 	/*
 	 * If we're aborting a subtransaction, we can immediately remove failed
 	 * XIDs from PGPROC's cache of running child XIDs.  We do that here for
@@ -1193,7 +1209,7 @@ RecordTransactionAbort(bool isSubXact)
 	 * main xacts, the equivalent happens just after this function returns.
 	 */
 	if (isSubXact)
-		XidCacheRemoveRunningXids(xid, nchildren, children);
+		XidCacheRemoveRunningXids(xid, nchildren, children, latestXid);
 
 	/* Reset XactLastRecEnd until the next transaction writes something */
 	if (!isSubXact)
@@ -1204,6 +1220,8 @@ RecordTransactionAbort(bool isSubXact)
 		pfree(rels);
 	if (children)
 		pfree(children);
+
+	return latestXid;
 }
 
 /*
@@ -1481,6 +1499,7 @@ static void
 CommitTransaction(void)
 {
 	TransactionState s = CurrentTransactionState;
+	TransactionId latestXid;
 
 	ShowTransactionState("CommitTransaction");
 
@@ -1552,7 +1571,7 @@ CommitTransaction(void)
 	/*
 	 * Here is where we really truly commit.
 	 */
-	RecordTransactionCommit();
+	latestXid = RecordTransactionCommit();
 
 	PG_TRACE1(transaction__commit, MyProc->lxid);
 
@@ -1560,47 +1579,8 @@ CommitTransaction(void)
 	 * Let others know about no transaction in progress by me. Note that
 	 * this must be done _before_ releasing locks we hold and _after_
 	 * RecordTransactionCommit.
-	 *
-	 * Note: MyProc may be null during bootstrap.
 	 */
-	if (MyProc != NULL)
-	{
-		if (TransactionIdIsValid(MyProc->xid))
-		{
-			/*
-			 * We must lock ProcArrayLock while clearing MyProc->xid, so
-			 * that we do not exit the set of "running" transactions while
-			 * someone else is taking a snapshot.  See discussion in
-			 * src/backend/access/transam/README.
-			 */
-			LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
-
-			MyProc->xid = InvalidTransactionId;
-			MyProc->lxid = InvalidLocalTransactionId;
-			MyProc->xmin = InvalidTransactionId;
-			MyProc->inVacuum = false;	/* must be cleared with xid/xmin */
-
-			/* Clear the subtransaction-XID cache too while holding the lock */
-			MyProc->subxids.nxids = 0;
-			MyProc->subxids.overflowed = false;
-
-			LWLockRelease(ProcArrayLock);
-		}
-		else
-		{
-			/*
-			 * If we have no XID, we don't need to lock, since we won't
-			 * affect anyone else's calculation of a snapshot.  We might
-			 * change their estimate of global xmin, but that's OK.
-			 */
-			MyProc->lxid = InvalidLocalTransactionId;
-			MyProc->xmin = InvalidTransactionId;
-			MyProc->inVacuum = false;	/* must be cleared with xid/xmin */
-
-			Assert(MyProc->subxids.nxids == 0);
-			Assert(MyProc->subxids.overflowed == false);
-		}
-	}
+	ProcArrayEndTransaction(MyProc, latestXid);
 
 	/*
 	 * This is all post-commit cleanup.  Note that if an error is raised here,
@@ -1824,20 +1804,8 @@ PrepareTransaction(void)
 	 * Let others know about no transaction in progress by me.	This has to be
 	 * done *after* the prepared transaction has been marked valid, else
 	 * someone may think it is unlocked and recyclable.
-	 *
-	 * We can skip locking ProcArrayLock here, because this action does not
-	 * actually change anyone's view of the set of running XIDs: our entry
-	 * is duplicate with the gxact that has already been inserted into the
-	 * ProcArray.
 	 */
-	MyProc->xid = InvalidTransactionId;
-	MyProc->lxid = InvalidLocalTransactionId;
-	MyProc->xmin = InvalidTransactionId;
-	MyProc->inVacuum = false;	/* must be cleared with xid/xmin */
-
-	/* Clear the subtransaction-XID cache too */
-	MyProc->subxids.nxids = 0;
-	MyProc->subxids.overflowed = false;
+	ProcArrayClearTransaction(MyProc);
 
 	/*
 	 * This is all post-transaction cleanup.  Note that if an error is raised
@@ -1921,6 +1889,7 @@ static void
 AbortTransaction(void)
 {
 	TransactionState s = CurrentTransactionState;
+	TransactionId latestXid;
 
 	/* Prevent cancel/die interrupt while cleaning up */
 	HOLD_INTERRUPTS();
@@ -1987,7 +1956,7 @@ AbortTransaction(void)
 	 * Advertise the fact that we aborted in pg_clog (assuming that we got as
 	 * far as assigning an XID to advertise).
 	 */
-	RecordTransactionAbort(false);
+	latestXid = RecordTransactionAbort(false);
 
 	PG_TRACE1(transaction__abort, MyProc->lxid);
 
@@ -1995,49 +1964,8 @@ AbortTransaction(void)
 	 * Let others know about no transaction in progress by me. Note that this
 	 * must be done _before_ releasing locks we hold and _after_
 	 * RecordTransactionAbort.
-	 *
-	 * Note: MyProc may be null during bootstrap.
 	 */
-	if (MyProc != NULL)
-	{
-		if (TransactionIdIsValid(MyProc->xid))
-		{
-			/*
-			 * We must lock ProcArrayLock while clearing MyProc->xid, so
-			 * that we do not exit the set of "running" transactions while
-			 * someone else is taking a snapshot.  See discussion in
-			 * src/backend/access/transam/README.
-			 */
-			LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
-
-			MyProc->xid = InvalidTransactionId;
-			MyProc->lxid = InvalidLocalTransactionId;
-			MyProc->xmin = InvalidTransactionId;
-			MyProc->inVacuum = false;	/* must be cleared with xid/xmin */
-			MyProc->inCommit = false;	/* be sure this gets cleared */
-
-			/* Clear the subtransaction-XID cache too while holding the lock */
-			MyProc->subxids.nxids = 0;
-			MyProc->subxids.overflowed = false;
-
-			LWLockRelease(ProcArrayLock);
-		}
-		else
-		{
-			/*
-			 * If we have no XID, we don't need to lock, since we won't
-			 * affect anyone else's calculation of a snapshot.  We might
-			 * change their estimate of global xmin, but that's OK.
-			 */
-			MyProc->lxid = InvalidLocalTransactionId;
-			MyProc->xmin = InvalidTransactionId;
-			MyProc->inVacuum = false;	/* must be cleared with xid/xmin */
-			MyProc->inCommit = false;	/* be sure this gets cleared */
-
-			Assert(MyProc->subxids.nxids == 0);
-			Assert(MyProc->subxids.overflowed == false);
-		}
-	}
+	ProcArrayEndTransaction(MyProc, latestXid);
 
 	/*
 	 * Post-abort cleanup.	See notes in CommitTransaction() concerning
@@ -3863,7 +3791,7 @@ AbortSubTransaction(void)
 									s->parent->subTransactionId);
 
 		/* Advertise the fact that we aborted in pg_clog. */
-		RecordTransactionAbort(true);
+		(void) RecordTransactionAbort(true);
 
 		/* Post-abort cleanup */
 		if (TransactionIdIsValid(s->transactionId))