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

On Thu, 3 Jul 2025 at 17:57, Anthonin Bonnefoy
<anthonin(dot)bonnefoy(at)datadoghq(dot)com> wrote:
>
> Thanks for the comments!
>
> On Wed, Jul 2, 2025 at 7:12 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
> > WAL files that have already been replayed can still be read again
> > for WAL archiving (if archive_mode = always) or for replication
> > (if the standby is acting as a streaming replication sender or
> > a logical replication publisher). No?
> >
> > Also, the WAL summarizer might read those WAL files as well.
>
> True, I've forgotten we could do this on standbys. For archive_mode
> and WAL summarizer, the check can be done with "XLogArchiveMode !=
> ARCHIVE_MODE_ALWAYS" and "!summarize_wal".
>
> For the replication, it looks a bit trickier. Checking if there's an
> active walsender process seems like a good approach but I haven't
> found an existing way to do this. Checking WalSndCtl->walsnds for used
> slots isn't great as this should stay in walsender_private.h. A better
> way would be to check how many elements there are in
> ProcGlobal->walsenderFreeProcs. If there's max_wal_sender elements in
> the list, then it means there's no active walsender processes. There's
> already HaveNFreeProcs that could provide this information, though
> it's currently only doing this for the freeProcs list. I've modified
> HaveNFreeProcs to take a proc_free_list type as argument so it can be
> used to get the number of free slots in walsenderFreeProcs.
>
> One possible impact of this approach is when the cascading replication
> stream starts (either first time or after a disconnect), the WAL files
> will need to be read from disk instead of being already in the page
> cache. Though I think that's a better default behaviour as I would
> expect that most replicas don't have cascading replication and
> removing closed WAL segments would benefit most replicas.
>
> Regards,
> Anthonin Bonnefoy

Hi!
Looking at v2. You need to add ProcFreeList to
`src/bin/pgindent/typedefs.list` to avoid extra-spacing.

> Checking WalSndCtl->walsnds for used
> slots isn't great as this should stay in walsender_private.h. A better
> way would be to check how many elements there are in
> ProcGlobal->walsenderFreeProcs. If there's max_wal_sender elements in
> the list, then it means there's no active walsender processes.

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?

Other than that, the patch looks good.

--
Best regards,
Kirill Reshke