Re: Reduce SyncRepLock contention on the commit path - Mailing list pgsql-hackers

17.08.2026 11:29, Vadim Ponomarev пишет:
> Hi hackers,
> 
> On a primary with synchronous replication, every commit that wrote WAL
> goes through SyncRepWaitForLSN(), and every standby reply goes through
> SyncRepReleaseWaiters().  Both take SyncRepLock exclusively, although
> much of their work does not need to happen under the lock.
> 
> The attached series moves that work out of the critical section.  The
> four patches are independent; only 0003 changes observable behaviour.
> 
> 0001 -- Wake released waiters after dropping the queue lock.
> 
> SyncRepWakeQueue() sets each released backend's latch while holding
> SyncRepLock.  Setting a latch may call kill(), so at high commit rates
> the walsender makes one syscall per released commit while committers
> queue on the same lock.
> 
> The patch collects released procs in a list and wakes them after dropping
> the lock, as ProcArrayGroupClearXid() already does with ProcArrayLock.
> The unlink, write barrier, and state update remain under the lock: a
> waiter reads syncRepState without it and must not see itself as completed
> while still on the queue.
> 
> 0002 -- Compute synced positions before taking the queue lock.
> 
> SyncRepReleaseWaiters() takes SyncRepLock before scanning the walsender
> slots for synced write, flush, and apply positions.  That scan takes a
> spinlock per slot, allocates memory, and sorts quorum results.
> 
> The patch moves the scan before the lock.  The consumers only move lsn[]
> forward, so a result that becomes stale while waiting for the lock simply
> does not advance it.  The patch also avoids the lock when the walsender
> is not a synchronous standby.
> 
> 0003 -- Release waiters once per drained batch of standby replies.
> 
> ProcessStandbyReplyMessage() calls SyncRepReleaseWaiters() for every
> reply, even when several replies are already waiting in the socket.
> Except for the last one, each pass then uses positions that the next
> message immediately replaces.
> 
> The patch marks a release as pending and runs it once after draining the
> socket.  The main risk is losing a pending release on an early exit: no
> other process will wake the committers acknowledged by that reply.
> 
> The goodbye, EOF, invalid and unexpected message paths all run the
> pending release before returning.  A normal standby shutdown sends its
> final reply and goodbye together, so that path matters in practice.
> Errors while parsing a later message, including a torn message, go
> through WalSndErrorCleanup(), which releases waiters after dropping the
> locks.
> 
> 0004 -- Skip the lock when the acknowledgement has already arrived.
> 
> This patch mirrors lsn[] in an atomic watermark and checks it before
> SyncRepWaitForLSN() takes the lock.  The watermark is updated under the
> lock immediately after lsn[].  Both only move forward, so a stale read
> may take the slow path unnecessarily but cannot skip a required wait.
> 
> I am least sure that 0004 is worth the extra shared state; see below.
> 
> Prior work
> ----------
> 
> 0001 was proposed in Thomas Munro's "Latches vs lwlock contention"
> thread as part of a general SetLatches() facility:
> 
>   https://www.postgresql.org/message-id/CA%2BhUKGKmO7ze0Z6WXKdrLxmvYa%3DzVGGXOO30MMktufofVwEm1A%40mail.gmail.com
> 
> The heavyweight-lock part of that work was committed in November 2024,
> but SetLatches() and its sync-rep use were not.  As far as I can tell,
> the sync-rep patch did not get a separate review.  I used a list local to
> syncrep.c to keep the change contained and avoid the open questions
> around buffers and allocation in the general facility.
> 
> There is also precedent for 0004.  Michael Paquier's 2e57790836c ("Fix
> race with synchronous_standby_names at startup", April 2025) reads
> WalSndCtl->sync_standbys_status without the lock.  The same monotonicity
> argument applies here; the LSN needs the atomics API because it is 64
> bits wide.
> 
> Measurements
> ------------
> 
> I compared devel master with the same master plus all four patches on
> two hosts connected by a dedicated 100 GbE link (RTT 0.11 ms):
> 
>   primary   4-socket Xeon Platinum 8580, 240 threads, 2 TB RAM, NVMe
>   standby   2-socket Xeon Gold 5320, 104 threads, 1 TB RAM, NVMe
> 
> The test used pgbench scale 2000, fillfactor 70, 750 clients,
> -M prepared, the built-in TPC-B script, and 10-minute runs with
> synchronous_commit = on, fsync and full_page_writes on.  postgres used
> two primary sockets and pgbench a third.
> 
> Each point started from the same prepared cluster, and the standby was
> rebuilt from a fresh base backup.  I ran three interleaved pairs,
> alternating base and patched:
> 
>   tps runs        122250/122591/123091 -> 134849/134447/132740
>   mean tps        122644 -> 134012                         +9.3%
>   mean latency    6.085 ms -> 5.558 ms                    -8.7%
>   failed transactions: none
> 
> The spread was 0.7% for base and 1.6% for patched.
> 
> pg_stat_activity samples taken every 5 seconds show the same effect.
> The queue lock and standby acknowledgement wait are both named SyncRep;
> wait_event_type separates them:
> 
>   LWLock/SyncRep samples   12222 -> 6912     -43%
>   IPC/SyncRep samples      20699 -> 14694    -29%
> 
> Per committed transaction, lock-wait samples fell from 0.0997 to 0.0516.
> The queue-lock wait was roughly halved while throughput rose by 9%.  The
> standby acknowledgement wait also fell by 35% per transaction, as
> committers stopped queueing for SyncRepLock before waiting for the
> standby.
> 
> To see which patches contributed, I also ran -DLWLOCK_STATS builds on a
> small single-host setup: 16 threads, scale 20, 128 clients, 30 seconds.
> Across two runs, base -> patched:
> 
>   walsender acquisitions/commit   0.900-1.106 -> 0.538-0.610
>   backend acquisitions/commit     0.9983-0.9995 -> 0.977-0.984
>   commits that blocked            9.34-10.20% -> 0.24-0.27%
> 
> At 32 clients, blocked acquisitions fell from 37809 to 362.
> 
> 0003 makes most of the difference: the walsender takes the lock roughly
> half as often.  0004 rarely takes its fast path, saving only 1.6-2.3% of
> backend acquisitions at 128 clients.  With synchronous_commit = on, it
> can only catch a commit if an acknowledgement for a later transaction
> happens to cover its LSN.
> 
> I also ran four interleaved, 60-second single-client pairs.  Mean
> latency was 0.760 ms on base and 0.762 ms patched, so deferring the
> release to the end of a one-reply drain showed no measurable delay.
> 
> Testing
> -------
> 
> 0003 adds src/test/recovery/t/056_syncrep_release.pl for two cases where
> a deferred release could be lost:
> 
> * A stopped walsender resumes after the standby has applied the commit
>   and shut down, then drains the final reply and goodbye together.
> 
> * An injection point after a drained reply stands in for a torn message.
>   The walreceiver is stopped while replay advances from WAL already on
>   disk, so the first reply after resume carries the apply position the
>   committer needs.
> 
> Notes
> -------
> 
> The SyncRepReleaseWaiters() call on configuration reload is outside the
> reply drain and is unchanged by 0003.
> 
> 0001 allocates a MaxBackends-sized wake list once per process in
> TopMemoryContext.  I am open to changing that if there is a better fit.
> 
> The patches are against 7e6e294e4e4.
> 
> 
> -------
> 
> Review would be especially helpful on two points:
> 
> * Did I miss any exit path from the reply drain that must run the
>   deferred release?
> 
> * Is 0004 worth its new atomic in WalSndCtlData for a 1.6-2.3% reduction
>   in backend lock acquisitions in these tests?
> 
> I plan to register 0001-0003 for the September CommitFest and drop 0004,
> unless there is a workload where its fast path is more useful.
> 
> Regards,
> Vadim Ponomarev

Good day, Vadim.

We've measured your patches and confirm they improve performance of
synchronous replication:
- with couple of 56 vcore virtual machines and pgbench running on replica,
250 clients, scale 2000, "TPC-B like" improved from 67.3kTPS to 70.4kTPS.
Which is quite impressive.

0001 i've found independently, so I fully share the idea. It really works.

0003 impressed me a lot. Great thing, imo!

0002 looks like "dirty-hack", but reading closely I found no issues:
- SyncRepGetCandidatesStandbys syncs by spinlocks on every walsender
- all WalSndCtl->lsn increases monotonically under lock.

0004 it really doesn't cost anything and gives some value. So let it be.

We didn't measure things one-by-one, but I suppose 0001 and 0003 gives most
of gain. Still other thing are useful as well, I believe 0002 and 0004 are
worth to be committed.

I've rebased patches and simplified a bit 0001 and 0004:
- 0001 already uses static variable. So why don't just make it file-wide
and use in SyncRepWakeQueue directly?
- 0004 - there is no need in separate lsn and lsn_published. Lets simply
convert lsn to atomic variable.
  And I've refactored condition under lock a bit to make it clear why test
against of just atomic WalSndCtl->lsn[mode] could be enough. It was a bit
non-obvious in the form it is in master branch.

And I've added 027_stream_regress_sync.pl in 0005 as tweaked copy of
027_stream_regress.pl to test synchronous replication under concurrent load.

-- 
regards
Yura Sokolov aka funny-falcon