Date: Mon, 31 Aug 2020 14:21:56 -0400
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Jaime Casanova <jaime.casanova@2ndQuadrant.com>
Subject: LogwrtResult contended spinlock
Message-ID: <20200831182156.GA3983@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="/04w6evG8XlLl3ft"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk


--/04w6evG8XlLl3ft
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Jaime Casanova recently reported a situation where pglogical replicating
from 64 POS sites to a single central (64-core) node, each with two
replication sets, causes XLog's info_lck to become highly contended
because of frequently reading LogwrtResult.  We tested the simple fix of
adding a new LWLock that protects LogwrtResult and LogwrtRqst; that
seems to solve the problem easily enough.

At first I wanted to make the new LWLock cover only LogwrtResult proper,
and leave LogwrtRqst alone.  However on doing it, it seemed that that
might change the locking protocol in a nontrivial way.  So I decided to
make it cover both and call it a day.  We did verify that the patch
solves the reported problem, at any rate.

-- 
Álvaro Herrera                PostgreSQL Expert, https://www.2ndQuadrant.com/

--/04w6evG8XlLl3ft
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0001-use-an-LWLock-rather-than-spinlock-for-Logwrt-Result.patch"

From 3eb871f235c1b6005ff5dc88561173e4e92c1314 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Mon, 17 Aug 2020 19:05:22 -0400
Subject: [PATCH] use an LWLock rather than spinlock for Logwrt{Result,Rqst}

---
 src/backend/access/transam/xlog.c        | 76 +++++++++++++-----------
 src/backend/storage/lmgr/lwlocknames.txt |  2 +-
 2 files changed, 43 insertions(+), 35 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 09c01ed4ae..d838739bc4 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -403,15 +403,13 @@ static XLogRecPtr RedoStartLSN = InvalidXLogRecPtr;
  * These structs are identical but are declared separately to indicate their
  * slightly different functions.
  *
- * To read XLogCtl->LogwrtResult, you must hold either info_lck or
- * WALWriteLock.  To update it, you need to hold both locks.  The point of
- * this arrangement is that the value can be examined by code that already
- * holds WALWriteLock without needing to grab info_lck as well.  In addition
- * to the shared variable, each backend has a private copy of LogwrtResult,
- * which is updated when convenient.
+ * To read/write XLogCtl->LogwrtResult, you must hold WALWriteResultLock.
+ * In addition to the shared variable, each backend has a private copy of
+ * LogwrtResult, which is updated when convenient.
  *
  * The request bookkeeping is simpler: there is a shared XLogCtl->LogwrtRqst
- * (protected by info_lck), but we don't need to cache any copies of it.
+ * (also protected by WALWriteResultLock), but we don't need to cache any
+ * copies of it.
  *
  * info_lck is only held long enough to read/update the protected variables,
  * so it's a plain spinlock.  The other locks are held longer (potentially
@@ -615,10 +613,7 @@ typedef struct XLogCtlData
 	pg_time_t	lastSegSwitchTime;
 	XLogRecPtr	lastSegSwitchLSN;
 
-	/*
-	 * Protected by info_lck and WALWriteLock (you must hold either lock to
-	 * read it, but both to update)
-	 */
+	/* Protected by WALWriteResultLock. */
 	XLogwrtResult LogwrtResult;
 
 	/*
@@ -1158,13 +1153,12 @@ XLogInsertRecord(XLogRecData *rdata,
 	 */
 	if (StartPos / XLOG_BLCKSZ != EndPos / XLOG_BLCKSZ)
 	{
-		SpinLockAcquire(&XLogCtl->info_lck);
 		/* advance global request to include new block(s) */
+		LWLockAcquire(WALWriteResultLock, LW_EXCLUSIVE);
 		if (XLogCtl->LogwrtRqst.Write < EndPos)
 			XLogCtl->LogwrtRqst.Write = EndPos;
-		/* update local result copy while I have the chance */
 		LogwrtResult = XLogCtl->LogwrtResult;
-		SpinLockRelease(&XLogCtl->info_lck);
+		LWLockRelease(WALWriteResultLock);
 	}
 
 	/*
@@ -2155,12 +2149,12 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, bool opportunistic)
 			if (opportunistic)
 				break;
 
-			/* Before waiting, get info_lck and update LogwrtResult */
-			SpinLockAcquire(&XLogCtl->info_lck);
+			/* Before waiting, update LogwrtResult */
+			LWLockAcquire(WALWriteResultLock, LW_EXCLUSIVE);
 			if (XLogCtl->LogwrtRqst.Write < OldPageRqstPtr)
 				XLogCtl->LogwrtRqst.Write = OldPageRqstPtr;
 			LogwrtResult = XLogCtl->LogwrtResult;
-			SpinLockRelease(&XLogCtl->info_lck);
+			LWLockRelease(WALWriteResultLock);
 
 			/*
 			 * Now that we have an up-to-date LogwrtResult value, see if we
@@ -2178,9 +2172,11 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, bool opportunistic)
 
 				WaitXLogInsertionsToFinish(OldPageRqstPtr);
 
-				LWLockAcquire(WALWriteLock, LW_EXCLUSIVE);
-
+				LWLockAcquire(WALWriteResultLock, LW_SHARED);
 				LogwrtResult = XLogCtl->LogwrtResult;
+				LWLockRelease(WALWriteResultLock);
+
+				LWLockAcquire(WALWriteLock, LW_EXCLUSIVE);
 				if (LogwrtResult.Write >= OldPageRqstPtr)
 				{
 					/* OK, someone wrote it already */
@@ -2426,7 +2422,9 @@ XLogWrite(XLogwrtRqst WriteRqst, bool flexible)
 	/*
 	 * Update local LogwrtResult (caller probably did this already, but...)
 	 */
+	LWLockAcquire(WALWriteResultLock, LW_SHARED);
 	LogwrtResult = XLogCtl->LogwrtResult;
+	LWLockRelease(WALWriteResultLock);
 
 	/*
 	 * Since successive pages in the xlog cache are consecutively allocated,
@@ -2663,13 +2661,14 @@ XLogWrite(XLogwrtRqst WriteRqst, bool flexible)
 	 * code in a couple of places.
 	 */
 	{
-		SpinLockAcquire(&XLogCtl->info_lck);
+		LWLockAcquire(WALWriteResultLock, LW_EXCLUSIVE);
 		XLogCtl->LogwrtResult = LogwrtResult;
+
 		if (XLogCtl->LogwrtRqst.Write < LogwrtResult.Write)
 			XLogCtl->LogwrtRqst.Write = LogwrtResult.Write;
 		if (XLogCtl->LogwrtRqst.Flush < LogwrtResult.Flush)
 			XLogCtl->LogwrtRqst.Flush = LogwrtResult.Flush;
-		SpinLockRelease(&XLogCtl->info_lck);
+		LWLockRelease(WALWriteResultLock);
 	}
 }
 
@@ -2684,8 +2683,10 @@ XLogSetAsyncXactLSN(XLogRecPtr asyncXactLSN)
 	XLogRecPtr	WriteRqstPtr = asyncXactLSN;
 	bool		sleeping;
 
-	SpinLockAcquire(&XLogCtl->info_lck);
+	LWLockAcquire(WALWriteResultLock, LW_SHARED);
 	LogwrtResult = XLogCtl->LogwrtResult;
+	LWLockRelease(WALWriteResultLock);
+	SpinLockAcquire(&XLogCtl->info_lck);
 	sleeping = XLogCtl->WalWriterSleeping;
 	if (XLogCtl->asyncXactLSN < asyncXactLSN)
 		XLogCtl->asyncXactLSN = asyncXactLSN;
@@ -2893,11 +2894,11 @@ XLogFlush(XLogRecPtr record)
 		XLogRecPtr	insertpos;
 
 		/* read LogwrtResult and update local state */
-		SpinLockAcquire(&XLogCtl->info_lck);
+		LWLockAcquire(WALWriteResultLock, LW_SHARED);
 		if (WriteRqstPtr < XLogCtl->LogwrtRqst.Write)
 			WriteRqstPtr = XLogCtl->LogwrtRqst.Write;
 		LogwrtResult = XLogCtl->LogwrtResult;
-		SpinLockRelease(&XLogCtl->info_lck);
+		LWLockRelease(WALWriteResultLock);
 
 		/* done already? */
 		if (record <= LogwrtResult.Flush)
@@ -2927,12 +2928,15 @@ XLogFlush(XLogRecPtr record)
 		}
 
 		/* Got the lock; recheck whether request is satisfied */
+		LWLockAcquire(WALWriteResultLock, LW_SHARED);
 		LogwrtResult = XLogCtl->LogwrtResult;
 		if (record <= LogwrtResult.Flush)
 		{
+			LWLockRelease(WALWriteResultLock);
 			LWLockRelease(WALWriteLock);
 			break;
 		}
+		LWLockRelease(WALWriteResultLock);
 
 		/*
 		 * Sleep before flush! By adding a delay here, we may give further
@@ -3043,10 +3047,10 @@ XLogBackgroundFlush(void)
 		return false;
 
 	/* read LogwrtResult and update local state */
-	SpinLockAcquire(&XLogCtl->info_lck);
+	LWLockAcquire(WALWriteResultLock, LW_SHARED);
 	LogwrtResult = XLogCtl->LogwrtResult;
 	WriteRqst = XLogCtl->LogwrtRqst;
-	SpinLockRelease(&XLogCtl->info_lck);
+	LWLockRelease(WALWriteResultLock);
 
 	/* back off to last completed page boundary */
 	WriteRqst.Write -= WriteRqst.Write % XLOG_BLCKSZ;
@@ -3128,7 +3132,9 @@ XLogBackgroundFlush(void)
 	/* now wait for any in-progress insertions to finish and get write lock */
 	WaitXLogInsertionsToFinish(WriteRqst.Write);
 	LWLockAcquire(WALWriteLock, LW_EXCLUSIVE);
+	LWLockAcquire(WALWriteResultLock, LW_SHARED);
 	LogwrtResult = XLogCtl->LogwrtResult;
+	LWLockRelease(WALWriteResultLock);
 	if (WriteRqst.Write > LogwrtResult.Write ||
 		WriteRqst.Flush > LogwrtResult.Flush)
 	{
@@ -3216,9 +3222,9 @@ XLogNeedsFlush(XLogRecPtr record)
 		return false;
 
 	/* read LogwrtResult and update local state */
-	SpinLockAcquire(&XLogCtl->info_lck);
+	LWLockAcquire(WALWriteResultLock, LW_SHARED);
 	LogwrtResult = XLogCtl->LogwrtResult;
-	SpinLockRelease(&XLogCtl->info_lck);
+	LWLockRelease(WALWriteResultLock);
 
 	/* check again */
 	if (record <= LogwrtResult.Flush)
@@ -7698,10 +7704,12 @@ StartupXLOG(void)
 
 	LogwrtResult.Write = LogwrtResult.Flush = EndOfLog;
 
+	LWLockAcquire(WALWriteResultLock, LW_EXCLUSIVE);
 	XLogCtl->LogwrtResult = LogwrtResult;
 
 	XLogCtl->LogwrtRqst.Write = EndOfLog;
 	XLogCtl->LogwrtRqst.Flush = EndOfLog;
+	LWLockRelease(WALWriteResultLock);
 
 	/*
 	 * Update full_page_writes in shared memory and write an XLOG_FPW_CHANGE
@@ -8406,9 +8414,9 @@ GetInsertRecPtr(void)
 {
 	XLogRecPtr	recptr;
 
-	SpinLockAcquire(&XLogCtl->info_lck);
+	LWLockAcquire(WALWriteResultLock, LW_SHARED);
 	recptr = XLogCtl->LogwrtRqst.Write;
-	SpinLockRelease(&XLogCtl->info_lck);
+	LWLockRelease(WALWriteResultLock);
 
 	return recptr;
 }
@@ -8420,9 +8428,9 @@ GetInsertRecPtr(void)
 XLogRecPtr
 GetFlushRecPtr(void)
 {
-	SpinLockAcquire(&XLogCtl->info_lck);
+	LWLockAcquire(WALWriteResultLock, LW_SHARED);
 	LogwrtResult = XLogCtl->LogwrtResult;
-	SpinLockRelease(&XLogCtl->info_lck);
+	LWLockRelease(WALWriteResultLock);
 
 	return LogwrtResult.Flush;
 }
@@ -11509,9 +11517,9 @@ GetXLogInsertRecPtr(void)
 XLogRecPtr
 GetXLogWriteRecPtr(void)
 {
-	SpinLockAcquire(&XLogCtl->info_lck);
+	LWLockAcquire(WALWriteResultLock, LW_SHARED);
 	LogwrtResult = XLogCtl->LogwrtResult;
-	SpinLockRelease(&XLogCtl->info_lck);
+	LWLockRelease(WALWriteResultLock);
 
 	return LogwrtResult.Write;
 }
diff --git a/src/backend/storage/lmgr/lwlocknames.txt b/src/backend/storage/lmgr/lwlocknames.txt
index 774292fd94..b4d114cfd5 100644
--- a/src/backend/storage/lmgr/lwlocknames.txt
+++ b/src/backend/storage/lmgr/lwlocknames.txt
@@ -50,6 +50,6 @@ MultiXactTruncationLock				41
 OldSnapshotTimeMapLock				42
 LogicalRepWorkerLock				43
 XactTruncationLock					44
-# 45 was XactTruncationLock until removal of BackendRandomLock
+WALWriteResultLock					45
 WrapLimitsVacuumLock				46
 NotifyQueueTailLock					47
-- 
2.20.1


--/04w6evG8XlLl3ft--