Fix archive restore race that could unlink WAL before rename

On 24 Jul 2026, at 20:29, Stepan Neretin <slpmcf@gmail.com> wrote:

Hi,

KeepFileRestoredFromArchive() used to unlink an existing WAL segment
before renaming the restored file into place on non-Windows. Between
those two steps the segment is missing from pg_wal, so a concurrent
walsender (or anything else looking at that path) can observe a gap.

On POSIX, durable_rename() already replaces the target atomically, so
the prior unlink is unnecessary. Windows still needs the
rename-to-.deletedN + unlink dance, so that path is unchanged.

Yup, the change seems correct to me.

The explicit unlink predates durable_rename(): it was added in 2012,
while this call site was switched to durable_rename() in 2016 by
1d4a0ab19a7.

The attached patch drops the non-Windows unlink and adds a TAP test that
uses an injection point between the old unlink site and the rename to
show the segment remains present.

The test needs some work. It fails on Windows [0]https://github.com/x4m/postgres_g/actions/runs/30458446069/job/90598673326. Test has precisely
zero comments. It is named after walsender, but never starts a walsender.
advance_wal() switches WAL segments, but it does not wait for the archiver.

It would be good to actually produce walsender error to demonstrate the race.

Thanks!

Best regards, Andrey Borodin.

[0]: https://github.com/x4m/postgres_g/actions/runs/30458446069/job/90598673326

On Thu, Jul 30, 2026 at 1:19 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:

On 24 Jul 2026, at 20:29, Stepan Neretin <slpmcf@gmail.com> wrote:

Hi,

KeepFileRestoredFromArchive() used to unlink an existing WAL segment
before renaming the restored file into place on non-Windows. Between
those two steps the segment is missing from pg_wal, so a concurrent
walsender (or anything else looking at that path) can observe a gap.

On POSIX, durable_rename() already replaces the target atomically, so
the prior unlink is unnecessary. Windows still needs the
rename-to-.deletedN + unlink dance, so that path is unchanged.

Yup, the change seems correct to me.

The explicit unlink predates durable_rename(): it was added in 2012,
while this call site was switched to durable_rename() in 2016 by
1d4a0ab19a7.

The attached patch drops the non-Windows unlink and adds a TAP test

that

uses an injection point between the old unlink site and the rename to
show the segment remains present.

The test needs some work. It fails on Windows [0]. Test has precisely
zero comments. It is named after walsender, but never starts a walsender.
advance_wal() switches WAL segments, but it does not wait for the archiver.

It would be good to actually produce walsender error to demonstrate the
race.

Thanks!

Best regards, Andrey Borodin.

[0]
https://github.com/x4m/postgres_g/actions/runs/30458446069/job/90598673326

Hi Andrey,
Thanks for the review. v2 attached with the test fixes you asked for.
Best regards,
Stepan Neretin

Hi Stepan,

Thanks for the v2 patch. I tested it locally and also instrumented
XLogSendPhysical() to
verify the walsender behavior.

In the current test, both SendRqsPtr and sentPtr are 0/00D00000.
Consequently, the walsender returnes at:

if (SendRqstPtr <= sentPtr)
{
WalSndCaughtUp = true;
return;
}

It never reaches WalSndSegmentOpen() and therefore does not attempt to open
the WAL segment involved in the race.

I also reintroduced the old unlink() behavior. The file-existence assertion
failed as expected, but the standby walsender still started successfully
and did not report a missing WAL segment.

Therefore, the test does verify that the segment remains present before the
rename, but the walsender portion does not currently exercise the
reported race. It may be better either to remove that portion or arrange
for SendRqstPtr > sentPtr so the walsender actually attempts to read the
segment.

Regards,

--
Ze Chen (Neil)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Wed, Aug 05, 2026 at 10:46:25AM +0800, Neil Chen wrote:

Therefore, the test does verify that the segment remains present before the
rename, but the walsender portion does not currently exercise the
reported race. It may be better either to remove that portion or arrange
for SendRqstPtr > sentPtr so the walsender actually attempts to read the
segment.

The test goes to great lengths to prove a point that can be guessed
directly by reading the code, and is enforced in the code:
durable_rename() ensures that we have no window once the unlink() is
gone.

IMO, there is no point in including the test at all; the position of
INJECTION_POINT() just serves at validating what durable_rename() is
designed to do: make a rename() atomic and offer durability
guarantees. I also suspect that the restore_command of the test is
going to break more than one buildfarm member.

One thing that may be worth adding is a mention that we care about
concurrent WAL senders, and that the race is still not closed on
Windows, though.

Spoiler: I don't care much about the WIN32 case but one could look at
something like ReplaceFile() if interested, as one idea? Just closing
the race on non-WIN32 has a good amount of value, and could always be
solved later. I have not looked if it's actually safe on the Postgres
side due to the handle requirements we have with share mode, just
throwing an idea.
--
Michael

On 5 Aug 2026, at 15:12, Michael Paquier <michael@paquier.xyz> wrote:

there is no point in including the test at all

There's no point in committing the test. There is a point in writing
test to demonstrate and describe bug with the test. That's why I
recommend writing test in a separate patch.

One thing that may be worth adding is a mention that we care about
concurrent WAL senders

+1.

Best regards, Andrey Borodin.

On Wed, Aug 05, 2026 at 04:07:02PM +0500, Andrey Borodin wrote:

On 5 Aug 2026, at 15:12, Michael Paquier <michael@paquier.xyz> wrote:

there is no point in including the test at all

There's no point in committing the test. There is a point in writing
test to demonstrate and describe bug with the test. That's why I
recommend writing test in a separate patch.

As my previous words may be a bit confusing. Proving a bug in the
shape of a test is super useful, even if the so-said test finishes by
not being committed.

One thing that may be worth adding is a mention that we care about
concurrent WAL senders

+1.

I'll do that, and likely apply the result. Another part is if this
should be backpatched or not, but I always find these recovery changes
quite stressing to do in stable branches, so for now I'd tend to just
do this change on HEAD, also due to the fact that we don't seem to
have reports about this tiny window being a problem in practice.
--
Michael

On Thu, Aug 06, 2026 at 07:16:43AM +0900, Michael Paquier wrote:

I'll do that, and likely apply the result. Another part is if this
should be backpatched or not, but I always find these recovery changes
quite stressing to do in stable branches, so for now I'd tend to just
do this change on HEAD, also due to the fact that we don't seem to
have reports about this tiny window being a problem in practice.

I have added a note about the window still existing on WIN32, and
applied the result as 1200dfd60c36 on HEAD. If somebody is interested
in the WIN32 case, feel free.
--
Michael

On Thu, Aug 6, 2026 at 8:39 AM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Aug 06, 2026 at 07:16:43AM +0900, Michael Paquier wrote:

I'll do that, and likely apply the result. Another part is if this
should be backpatched or not, but I always find these recovery changes
quite stressing to do in stable branches, so for now I'd tend to just
do this change on HEAD, also due to the fact that we don't seem to
have reports about this tiny window being a problem in practice.

I have added a note about the window still existing on WIN32, and
applied the result as 1200dfd60c36 on HEAD.

Thanks for working on this!

This isn't directly related to this commit, but while reading the modified code
I found a small pre-existing issue in the WIN32 path.

In KeepFileRestoredFromArchive(), if unlink(oldpath) fails, the error message
reports xlogfpath, even though the file being removed is actually oldpath:

if (unlink(oldpath) != 0)
ereport(FATAL,
(errcode_for_file_access(),
errmsg("could not remove file \"%s\": %m",
xlogfpath)));

Should we fix that as well?

Regards,

--
Fujii Masao

On Thu, Aug 06, 2026 at 10:43:21AM +0900, Fujii Masao wrote:

This isn't directly related to this commit, but while reading the modified code
I found a small pre-existing issue in the WIN32 path.

In KeepFileRestoredFromArchive(), if unlink(oldpath) fails, the error message
reports xlogfpath, even though the file being removed is actually oldpath:

if (unlink(oldpath) != 0)
ereport(FATAL,
(errcode_for_file_access(),
errmsg("could not remove file \"%s\": %m",
xlogfpath)));

Should we fix that as well?

Eh. Yes, that should be adjusted. Perhaps you would like to do so
yourself. I don't really think it's worth bothering beyond HEAD. If
you think differently, of course feel free.
--
Michael