v4-0002-review.patch

From 126d2ab89716e93325f98203877df2d8fdbce51f Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@2ndquadrant.com>
Date: Sat, 4 Nov 2023 17:53:16 +0100
Subject: [PATCH v4 2/2] review

---
 src/backend/access/heap/vacuumlazy.c |   2 +-
 src/backend/access/transam/xlog.c    | 103 ++++++++++++++++++++-------
 src/backend/replication/syncrep.c    |  18 +++--
 src/backend/utils/misc/guc_tables.c  |   6 +-
 src/include/access/xlog.h            |   2 +-
 src/include/executor/instrument.h    |   2 +-
 src/include/miscadmin.h              |   2 +-
 7 files changed, 97 insertions(+), 38 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index d9cc06f620..123539d0fc 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -767,7 +767,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
 							 (long long) walusage.wal_fpi,
 							 (unsigned long long) walusage.wal_bytes);
 			if(walusage.wal_throttled > 0)
-				appendStringInfo(&buf, _("%lld times throttled"), (long long) walusage.wal_throttled);
+				appendStringInfo(&buf, _("%lld times throttled\n"), (long long) walusage.wal_throttled);
 			else
 				appendStringInfo(&buf, _("\n"));
 
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 3947f889de..7a549f8c3f 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1,5 +1,4 @@
 /*-------------------------------------------------------------------------
-
  *
  * xlog.c
  *		PostgreSQL write-ahead log manager
@@ -111,7 +110,6 @@
 #include "utils/timestamp.h"
 #include "utils/varlena.h"
 
-
 extern uint32 bootstrap_data_checksum_version;
 
 /* timeline ID to be used when bootstrapping */
@@ -142,7 +140,7 @@ int			wal_retrieve_retry_interval = 5000;
 int			max_slot_wal_keep_size_mb = -1;
 int			wal_decode_buffer_size = 512 * 1024;
 bool		track_wal_io_timing = false;
-int		synchronous_commit_wal_throttle_threshold = 0; /* kb */
+int			wal_throttle_threshold = 0;
 
 #ifdef WAL_DEBUG
 bool		XLOG_DEBUG = false;
@@ -260,6 +258,8 @@ static int	LocalXLogInsertAllowed = -1;
  *
  * XactLastThrottledRecEnd points to the last XLOG record that should be throttled
  * as the additional WAL records could be generated before processing interrupts.
+ *
+ * XXX I'm not sure I understand what "record to be throttled" means?
  */
 XLogRecPtr	ProcLastRecPtr = InvalidXLogRecPtr;
 XLogRecPtr	XactLastRecEnd = InvalidXLogRecPtr;
@@ -657,6 +657,12 @@ static bool holdingAllLocks = false;
 static MemoryContext walDebugCxt = NULL;
 #endif
 
+/*
+ * Amount of WAL inserted in this backend since eithe the transaction
+ * start or throttle point (where we reset the counter to 0).
+ *
+ * XXX Not sure it should refer to "backend", it's really about xact, no?
+ */
 uint32	backendWalInserted = 0;
 
 static void CleanupAfterArchiveRecovery(TimeLineID EndOfLogTLI,
@@ -1085,17 +1091,33 @@ XLogInsertRecord(XLogRecData *rdata,
 		pgWalUsage.wal_records++;
 		pgWalUsage.wal_fpi += num_fpi;
 
-		/* WAL throttling: we can slow down (some) backends generating a lot of WAL
-		 * in syncrep scenario by waiting for standby confirmation. This allows
-		 * prioritzation of other backends over this backend in bandwidth constrained
-		 * WAN scenarios. Such throttled down backends are going to be visible with
-		 * "SyncRepThrottled" wait event.
+		/*
+		 * Decide if we need to throttle this backend, so that it does not write
+		 * WAL too fast, causing lag against the sync standby (which in turn
+		 * increases latency for standby confirmations). We may be holding locks
+		 * and blocking interrupts here, so we only make the decision, but the
+		 * wait (for sync standby confirmation) happens elsewhere.
+		 *
+		 * The throttling is applied only to large transactions (producing more
+		 * than wal_throttle_threshold kilobytes of WAL). Throttled backends
+		 * can be identified by a new wait event SYNC_REP_THROTTLED.
+		 *
+		 * Small transactions (by amount of produced WAL) are still subject to
+		 * the sync replication, so the same wait happens at commit time.
+		 *
+		 * XXX Not sure this is the right place for a comment explaining how the
+		 * throttling works. This place is way too low level, and rather far from
+		 * the place where the wait actually happens.
+		 *
+		 * XXX Should this be done even if XLogDelayPending is already set? Maybe
+		 * that should only update XactLastThrottledRecEnd, withoug incrementing
+		 * the pgWalUsage.wal_throttled counter?
 		 */
 		backendWalInserted += rechdr->xl_tot_len;
-		if ((synchronous_commit == SYNCHRONOUS_COMMIT_REMOTE_APPLY ||
-			synchronous_commit == SYNCHRONOUS_COMMIT_REMOTE_WRITE) &&
-			synchronous_commit_wal_throttle_threshold > 0 &&
-			backendWalInserted > synchronous_commit_wal_throttle_threshold * 1024L)
+
+		if ((synchronous_commit >= SYNCHRONOUS_COMMIT_REMOTE_WRITE) &&
+			(wal_throttle_threshold > 0) &&
+			(backendWalInserted >= wal_throttle_threshold * 1024L))
 		{
 			XactLastThrottledRecEnd = XactLastRecEnd;
 			InterruptPending = true;
@@ -2636,7 +2658,22 @@ XLogFlush(XLogRecPtr record)
 		return;
 	}
 
-	/* reset WAL throttling bytes counter */
+	/*
+	 * Reset WAL throttling bytes counter.
+	 *
+	 * XXX I think this is somewhat wrong. The LSN we want to flush may be
+	 * somewhere in the past, before some (most) of the generated WAL. For
+	 * example assume the backend just wrote 1MB, but then we ask for flush
+	 * with a position 1MB back. Most of the generated WAL is likely after
+	 * the flushed LSN, yet we're setting this to 0. (I don't know how common
+	 * such situation is, probably not very.)
+	 *
+	 * I don't think we can / should track amount of WAL for arbitrary LSN
+	 * values, but maybe we should track the last flush LSN position, and
+	 * then linearly approximate the WAL.
+	 *
+	 * XXX For the approximation we should probably use LogwrtResult.Flush.
+	 */
 	backendWalInserted = 0;
 
 	/* Quick exit if already known flushed */
@@ -9142,25 +9179,41 @@ SetWalWriterSleeping(bool sleeping)
 	SpinLockRelease(&XLogCtl->info_lck);
 }
 
-
 /*
- * Called from ProcessMessageInterrupts() to avoid waiting while
- * being in critical section. Performing those directly from XLogInsertRecord()
- * would cause locks to be held for longer duration.
+ * HandleXLogDelayPending
+ *		Throttle backends generating large amounts of WAL.
+ *
+ * The throttling is implemented by waiting for a sync replica confirmation for
+ * a convenient LSN position. In particular, we do not wait for the current LSN,
+ * which may be in a partially filled WAL page (and we don't want to write this
+ * one out - we'd have to write it out again, causing write amplification).
+ * Instead, we move back to the last fully WAL page.
+ *
+ * Called from ProcessMessageInterrupts() to avoid syncrep waits in XLogInsert(),
+ * which happens in critical section and with blocked interrupts (so it would be
+ * impossible to cancel the wait if it gets stuck). Also, there may be locks held
+ * and we don't want to hold them longer just because of the wait.
+ *
+ * XXX Andres suggested we actually go back a couple pages, to increase the
+ * probability the LSN was already flushed (obviously, this depends on how much
+ * lag we allow).
+ *
+ * XXX Not sure why we use XactLastThrottledRecEnd and not simply XLogRecEnd?
  */
 void
 HandleXLogDelayPending()
 {
-	/* flush only up to the last fully filled page to avoid repeating flushing
-	 * of the same page multiple times */
-	XLogRecPtr 	LastFullyWrittenXLogPage = XactLastThrottledRecEnd -
-		(XactLastThrottledRecEnd % XLOG_BLCKSZ);
+	XLogRecPtr 	lsn;
+
+	/* calculate last fully filled page */
+	lsn = XactLastThrottledRecEnd - (XactLastThrottledRecEnd % XLOG_BLCKSZ);
 
-	Assert(synchronous_commit_wal_throttle_threshold > 0);
-	Assert(backendWalInserted > synchronous_commit_wal_throttle_threshold * 1024L);
+	Assert(wal_throttle_threshold > 0);
+	Assert(backendWalInserted >= wal_throttle_threshold * 1024L);
 	Assert(XactLastThrottledRecEnd != InvalidXLogRecPtr);
 
-	XLogFlush(LastFullyWrittenXLogPage);
-	SyncRepWaitForLSN(LastFullyWrittenXLogPage, false);
+	XLogFlush(lsn);
+	SyncRepWaitForLSN(lsn, false);
+
 	XLogDelayPending = false;
 }
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index e0fd6cfb19..152c51fe29 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -144,8 +144,11 @@ static bool SyncRepQueueIsOrderedByLSN(int mode);
  * represents a commit record.  If it doesn't, then we wait only for the WAL
  * to be flushed if synchronous_commit is set to the higher level of
  * remote_apply, because only commit records provide apply feedback.
+ *
+ * This may be called either when waiting for PREPARE/COMMIT, of because of WAL
+ * throttling (in which case the flag XLogDelayPending is set to true). We use
+ * different wait events for these cases.
  */
-
 void
 SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
 {
@@ -231,7 +234,7 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
 	for (;;)
 	{
 		int			rc;
-		uint32			wait_event;
+		uint32		wait_event;
 
 		/* Must reset the latch before testing state. */
 		ResetLatch(MyLatch);
@@ -285,18 +288,21 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
 			break;
 		}
 
-		/* XLogDelayPending flag that is used here is being reset afterwards in
-		 * in HandleXLogDelayPending()
+		/*
+		 * XLogDelayPending means this syncrep wait happens because of WAL
+		 * throttling. The flag is reset in HandleXLogDelayPending() later.
 		 */
-		if(XLogDelayPending == true)
+		if(XLogDelayPending)
 			wait_event = WAIT_EVENT_SYNC_REP_THROTTLED;
 		else
 			wait_event = WAIT_EVENT_SYNC_REP;
+
 		/*
 		 * Wait on latch.  Any condition that should wake us up will set the
 		 * latch, so no need for timeout.
 		 */
-		rc = WaitLatch(MyLatch, WL_LATCH_SET | WL_POSTMASTER_DEATH, -1, wait_event);
+		rc = WaitLatch(MyLatch, WL_LATCH_SET | WL_POSTMASTER_DEATH, -1,
+					   wait_event);
 
 		/*
 		 * If the postmaster dies, we'll probably never get an acknowledgment,
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 5ebd269304..0e6bd25a32 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2883,13 +2883,13 @@ struct config_int ConfigureNamesInt[] =
 	},
 
 	{
-		{"synchronous_commit_wal_throttle_threshold", PGC_USERSET, REPLICATION_SENDING,
+		{"wal_throttle_after", PGC_USERSET, REPLICATION_SENDING,
 			gettext_noop("Sets the maximum amount of WAL in kilobytes a backend generates "
-						"which it waits for synchronous commit confiration even without commit"),
+						 " before waiting for sync standby, to limit the replication lag."),
 			NULL,
 			GUC_UNIT_KB
 		},
-		&synchronous_commit_wal_throttle_threshold,
+		&wal_throttle_threshold,
 		0, 0, MAX_KILOBYTES,
 		NULL, NULL, NULL
 	},
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 895ca62ca6..9d1175edab 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -54,7 +54,7 @@ extern PGDLLIMPORT char *wal_consistency_checking_string;
 extern PGDLLIMPORT bool log_checkpoints;
 extern PGDLLIMPORT bool track_wal_io_timing;
 extern PGDLLIMPORT int wal_decode_buffer_size;
-extern PGDLLIMPORT int synchronous_commit_wal_throttle_threshold;
+extern PGDLLIMPORT int wal_throttle_threshold;
 extern PGDLLIMPORT uint32 backendWalInserted;
 
 extern PGDLLIMPORT int CheckPointSegments;
diff --git a/src/include/executor/instrument.h b/src/include/executor/instrument.h
index 7a06ef3258..58eca5b7a1 100644
--- a/src/include/executor/instrument.h
+++ b/src/include/executor/instrument.h
@@ -53,7 +53,7 @@ typedef struct WalUsage
 	int64		wal_records;	/* # of WAL records produced */
 	int64		wal_fpi;		/* # of WAL full page images produced */
 	uint64		wal_bytes;		/* size of WAL records produced */
-	int64		wal_throttled;		/* # of times WAL throttling was engaged*/
+	int64		wal_throttled;		/* # of times WAL throttling was engaged */
 } WalUsage;
 
 /* Flag bits included in InstrAlloc's instrument_options bitmask */
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index aa56eeae3b..44641e0e4c 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -95,7 +95,7 @@ extern PGDLLIMPORT volatile sig_atomic_t IdleSessionTimeoutPending;
 extern PGDLLIMPORT volatile sig_atomic_t ProcSignalBarrierPending;
 extern PGDLLIMPORT volatile sig_atomic_t LogMemoryContextPending;
 extern PGDLLIMPORT volatile sig_atomic_t IdleStatsUpdateTimeoutPending;
-/* this doesn't need to be volatile sig_atomic_t as it set in XLogInsertRecord() */
+/* doesn't need to be volatile sig_atomic_t as it's not set by signal handler */
 extern PGDLLIMPORT bool XLogDelayPending;
 
 extern PGDLLIMPORT volatile sig_atomic_t CheckClientConnectionPending;
-- 
2.41.0