Re: [Patch] Batch fsyncs when recycling WAL segments

Hi Ayush,
Thanks for working on these performance enhancements.

Denis Smirnov (added to cc-list) and I did an initial code-review of
this patch together. We kept our focus on 0001 since its scope is
simpler and it should be able to go in independently of 0002. We’ll put
up some feedback on 0002 separately, since there’s some design aspects
to deliberate on.

0001 is directionally correct: the `.done' ordering is correct, WIN32
`.deleted' path is kept and restartpoints on a standby take the same
path.

The main concern for 0001 is in the `LOG' error-level used and risks
around data-loss.

A: Concerns on `LOG' error level leading to possible data-loss
══════════════════════════════════════════════════════════════

[ Rationale within v4-0001 commit message ]
> Keep the existing LOG-level failure policy. If the batch directory
> fsync fails, retain the archive-status files rather than cleaning
> them up or taking down the server.

v4-0001 intentionally kept the `LOG' and it matches master’s per-file
`durable_unlink(..., LOG)'. The removals themselves are harmless if the
fsync fails, since a resurrected old segment is just removed again.

However, in the case of a fsync error, it (the error) would belong to
the whole directory and not just to the `unlink'. The concern is that
the directory fsync can report an error belonging to a concurrent
segment installation and if the checkpointer would log it at `LOG', then
the installing backend’s own fsync may succeed and commits written into
that segment could be lost on power failure.

Whether this can happen depends on the filesystem (journaling
filesystems usually go read-only on a metadata I/O error), so it’s more
of a reason for caution instead of a demonstrated bug. Master has the
same exposure to this risk via `durable_unlink(..., LOG)', but since
0001 rewrites this call, routing it through `data_sync_elevel(ERROR)'
seems to be the safer choice (i.e. calling `fsync_fname(XLOGDIR, true)'
and dropping the early-return). A `PANIC' by default does take the
server down, but after fsyncgate that’s the accepted trade-off.

┌────
│  * Any code that reports a failure from fsync() or related functions should
│  * filter the error level with this function.
└────
Listing 1: partial-header for `data_sync_elevel', which prescribes the
default behaviour

A pattern-discussion for a separate thread
──────────────────────────────────────────

Denis pointed out a pattern where `durable_rename()' /
`durable_unlink()' use one elevel both for the `rename' / `unlink'
failing and for the fsync inside them failing. Callers chose `LOG' /
`DEBUG1' / `WARNING' for the former (e.g. 7f2a10fecd8 made
recycle/removal failures non-fatal because of Windows file locks), and
fsync failures silently inherited it, since 9ccdd7f66e3 didn’t revisit
some of these call-sites.

There’s a case to be made about splitting the two levels — will defer
the discussion on this topic to its own thread. Denis has an initial
list of 6 call-sites that follow this pattern. The `pg_wal' ones matter
since they depend on durability.

B.  The `RemoveNonParentXlogFiles' path
═══════════════════════════════════════

0001 leaves `RemoveNonParentXlogFiles()' unbatched as “not part of the
normal checkpoint removal pass”.

A 2017 thread [1] (“Speed up the removal of WAL files”) targeted that
path, since its per-file `pg_wal fsyncs' add to failover time, and it
was returned with feedback rather than refuted.

Was leaving `RemoveNonParentXlogFiles' out a scope choice, or is there a
safety reason?

One difference I can see: that path removes old-timeline segments, some
of which may have no archive status file. So, a lost unlink could bring
back a file that the next checkpoint could mark `.ready', and the
`.done' ordering that makes 0001 safe wouldn’t cover it.

If it’s just scope, maybe a sentence in the commit message saying so
would help.

C. Nits
═══════

C1: docstring should include `cleanup_names'
────────────────────────────────────────────

`RemoveXlogFile()' ’s header comment should also document
`cleanup_names'. It should mention that `NULL' means durable-now and
non-`NULL' means the caller fsyncs and then cleans up.

C2: `NIL' and `NULL'
────────────────────

`RemoveXlogFile()' has 2 callers that differ by their level of
indirection. `RemoveOldXlogFiles' passes `&cleanup_names' (a pointer to
a list that starts as `NIL'), and `RemoveNonParentXlogFiles()' passes
`NULL'. Since `List **' doubles as a mode-switch, consider documenting
that to make it more explicit or bringing 0002’s `bool batch' into 0001.

Best,
Ritesh

Footnotes
─────────

[1] : “Speed up the removal of WAL files” [thread]
(<https://www.postgresql.org/message-id/flat/0A3221C70F24FB45833433255569204D1F81B0C8%40G01JPEXMBYT05>)

On Friday, November 13th, 2026 at 8:54 AM, Ayush Tiwari <ayushtiwari(dot)slg01(at)gmail(dot)com> wrote:

> Hi,
>

> While working on [1], I read on the "Pre-allocating WAL files" thread [2]
> that the fsyncs we do when recycling WAL segments are a real bottleneck,
> and Andres suggested batching them: rename a batch first, then fsync the
> files and pg_wal once, instead of the current per-segment
> durable_rename() in InstallXLogFileSegment(). The attached patch does
> that. (Andres and Nathan, who were on that thread, are cc'd.)
>

> The tricky part was durability. As soon as a segment is renamed it can
> be handed to the WAL write path, but the rename isn't on disk yet, and
> issue_xlog_fsync() only fsyncs the segment file, never the directory.
> So if a backend writes a commit into a just-recycled segment and we
> crash before the batched fsync, the rename can be lost.
>

> What I ended up with is a small "durability frontier" in XLogCtl (a
> timeline and the highest segment whose rename is known durable). The
> checkpointer advances it after the batched fsync, and XLogFileInit()
> checks it before letting anyone write into a segment; if the segment
> isn't covered yet it just fsyncs the file and pg_wal itself. Normally
> the checkpointer stays ahead, so that fallback basically never fires(?) it's
> only there to close the crash window. An injection point and a TAP test
> force that window open and confirm a committed row survives an immediate
> crash.
>

> Rough numbers: a WAL-heavy workload recycling ~44 segments per
> checkpoint (min_wal_size=1GB, max_wal_size=2GB, wal_recycle=on,
> fsync=on), ext4 on an Azure disk, 14 checkpoints with the first dropped:
>

> median mean min max
> master 138.6 ms 143 ms 123 ms 171 ms
> patch 27.3 ms 28 ms 25 ms 39 ms
>

> ~5x here, and a lot steadier: master pays ~44 flushes every checkpoint,
> the patch pays one.
>

> Is the frontier + write-path check the right shape for this, or is there a
> simpler invariant that keeps the write path from ever reaching a
> not-yet-durable recycled segment?
>

> Thoughts?
>

> Regards,
> Ayush
>

> [1] https://www.postgresql.org/message-id/flat/CAJTYsWXAPabSWBeT=bqJD+6w0D4-ozvoVoH7KiFUncM88bsQOg(at)mail(dot)gmail(dot)com
> [2] https://www.postgresql.org/message-id/745zvagaf6tfn2zbvtmqvxsr6kmybjbzcw6rsm55jnql4233je%40tudeipoewruo