Re: Fix archive restore race that could unlink WAL before rename

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

>
>
> > On 24 Jul 2026, at 20:29, Stepan Neretin <slpmcf(at)gmail(dot)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