Subject: Re: Don't keep closed WAL segment in page cache after replay

From: Anthonin Bonnefoy <[email-masked]>
Date: Wed, 18 Feb 2026 10:51:59 +0100

Thanks for the review!

On Tue, Feb 17, 2026 at 9:38 AM Kirill Reshke <[email-masked]> wrote:
> This does not immediately strike me as good reasoning. We have, for
> example, pg_stat_get_wal_senders (or WalSndInitStopping from
> walsenders.h) function which accesses exactly WalSndCtl->walsnds.
> Why don't we simply have another utility function that will return the
> number of active walsenders?

That's true it is an option. I've switched to this approach and
created a WalSndRunning. We only need to know if there's at least one
wal sender running, no need to have a precise number.

I've also added StandbyMode as a condition to restrict this to
replicas. XLogPageRead may be used by the primary when starting up,
and will likely re-read the WAL so releasing cached pages should be
avoided on the primary.

---

From: Anthonin Bonnefoy <[email-masked]>
Date: Tue, 3 Mar 2026 15:13:37 +0100

On Mon, Mar 2, 2026 at 9:15 AM Hüseyin Demir <[email-masked]> wrote:
> The idea looks good and efficient albeit I have some feedback. The first one is about logical replication slot.
>
> Inside the patch, it checks if there is an active walsender process. Is it possible to create a replication slot and wait until a subscriber will connect it. During this time due to patch PostgreSQL will close the WAL segments on the memory and once the subscriber connects it has to read the WAL files from disk. But it's a trade-off and can be decided by others too.

Good point. This is also the case for physical replication slots, if
there's at least one used replication slot, then it's very likely a
walsender will start at some point and would need to read the WAL. And
having to read a large amount of WAL files from disk is likely not
desirable, so it's probably better to add this as a condition.

> Secondly, when it comes to using spinlock to check the running walsender processes it can lead inefficient recovery process. Because assuming that a database with max_wal_sender set to 20+ and producing more than 4-5TB WAL in a day it can cause additional +100-200 spinlocks each second on walreceiver.  Put simply, WalSndRunning() scans every walsender slot with spinlocks on every segment switch, contending with all active walsenders updating their own slots. On high-throughput standbys this creates unnecessary cross-process spinlock contention in the recovery hot path — the
> exact path that should be as lean as possible for fast replay. Maybe you can implement a single pg_atomic_uint32 counter in WalSndCtlData and achieve the same result with zero contention.

True. I think I've assumed that a segment closing is rare enough that
it would be fine to go through all walsenders, but there are certainly
clusters that can generate a large amount of segments.

I've updated the patch with the suggested approach:
- a new atomic counter tracking the number of active walsenders
- a similar atomic counter for the number of used replication slots.
Otherwise, we would have a similar issue of going through all
max_replication_slots to check if one is used.
- Both are now used as condition to send POSIX_FADV_DONTNEED or not

Regards,
Anthonin Bonnefoy

---

From: Japin Li <[email-masked]>
Date: Wed, 04 Mar 2026 09:40:30 +0800

Hi, Anthonin

Date: Wed, 04 Mar 2026 09:37:27 +0800

On Tue, 03 Mar 2026 at 15:13, Anthonin Bonnefoy <[email-masked]> wrote:
>> Inside the patch, it checks if there is an active walsender
>> process. Is it possible to create a replication slot and wait until
>> a subscriber will connect it. During this time due to patch
>> PostgreSQL will close the WAL segments on the memory and once the
>> subscriber connects it has to read the WAL files from disk. But it's
>> a trade-off and can be decided by others too.
>
>> Secondly, when it comes to using spinlock to check the running
>> walsender processes it can lead inefficient recovery
>> process. Because assuming that a database with max_wal_sender set to
>> 20+ and producing more than 4-5TB WAL in a day it can cause
>> additional +100-200 spinlocks each second on walreceiver.  Put
>> simply, WalSndRunning() scans every walsender slot with spinlocks on
>> every segment switch, contending with all active walsenders updating
>> their own slots. On high-throughput standbys this creates
>> unnecessary cross-process spinlock contention in the recovery hot
>> path — the

I noticed two different comment styles in the v4 patch:

1.
+/*
+ * Return the number of used replication slots
+ */
+int
+ReplicationSlotNumUsed(void)

2.
+/*
+ * Returns the number of active walsender processes
+ */
+int
+WalSndNumActive(void)

Both "Return ..." and "Returns ..." styles exist in the PostgreSQL codebase,
but within the same patch, would it be better to use a consistent style?

I'd like to use the imperative/singular form.  What do you think?

>
> [2. text/x-diff; v4-0001-Don-t-keep-closed-WAL-segments-in-page-cache-afte.patch]...

-- 
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.

---

From: Anthonin Bonnefoy <[email-masked]>
Date: Wed, 4 Mar 2026 08:38:24 +0100

> but within the same patch, would it be better to use a consistent style?

That makes sense, I've updated the comment. I think I usually look
around the code to copy the comments' style, but as you said, both are
present in PostgreSQL codebase.

Regards,
Anthonin Bonnefoy

---

From: Andres Freund <[email-masked]>
Date: Wed, 4 Mar 2026 10:55:58 -0500

Hi,

On 2026-03-04 08:38:24 +0100, Anthonin Bonnefoy wrote:
> From ad0a3cfe10bdd2cccc4274849c4a77898b06e13c Mon Sep 17 00:00:00 2001
> From: Anthonin Bonnefoy <[email-masked]>
> Date: Wed, 2 Jul 2025 09:58:52 +0200
> Subject: Don't keep closed WAL segments in page cache after replay
> 
> On a standby, the recovery process reads the WAL segments, applies
> changes and closes the segment. When closed, the segments will still be
> in page cache memory until they are evicted due to inactivity. The
> segments may be re-read if archive_mode is set to always, wal_summarizer
> is enabled or if the standby is used for replication and has an active
> walsender.
> 
> The presence of a replication slots is also a likely indicator that
> a walsender will be started, and need to read the WAL segments.
> 
> Outside of those circumstances, the WAL segments won't be re-read and
> keeping them in the page cache generates unnecessary memory pressure.
> A POSIX_FADV_DONTNEED is sent before closing a replayed WAL segment to
> immediately free any cached pages.

I am quite sceptical that this is a good idea.

Have you actually measured benefits? I skimmed the thread and didn't see
anything.  It's pretty cheap for the kernel to replace a clean page from the
page cache with different content.

If you [crash-]restart the replica this will make it way more expensive.  If
you have twophase commits where we need to read 2PC details from the WAL, this
will make it more expensive.  If somebody takes a base backup, this ...

I think you'd have to have pretty convincing benchmarks showing that this is a
good idea before we should even remotely consider applying this.

Greetings,

Andres Freund