From 126d2ab89716e93325f98203877df2d8fdbce51f Mon Sep 17 00:00:00 2001 From: Tomas Vondra 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