On Thu, Jul 30, 2026 at 1:19 PM Andrey Borodin <[email protected]> wrote:
> > > > On 24 Jul 2026, at 20:29, Stepan Neretin <[email protected]> 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
From 0c87d567c621f467710008093d1d9b0f3bfd96b5 Mon Sep 17 00:00:00 2001 From: snppg <[email protected]> Date: Tue, 4 Aug 2026 21:43:07 +0700 Subject: [PATCH v2] Fix archive restore race that could unlink WAL before rename. --- src/backend/access/transam/xlogarchive.c | 11 +- src/test/recovery/meson.build | 1 + .../t/056_archive_restore_walsender_race.pl | 126 ++++++++++++++++++ 3 files changed, 132 insertions(+), 6 deletions(-) create mode 100644 src/test/recovery/t/056_archive_restore_walsender_race.pl diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c index 9a0c8097cb1..4e81eab85e5 100644 --- a/src/backend/access/transam/xlogarchive.c +++ b/src/backend/access/transam/xlogarchive.c @@ -31,6 +31,7 @@ #include "replication/walsender.h" #include "storage/fd.h" #include "storage/ipc.h" +#include "utils/injection_point.h" #include "utils/wait_event.h" /* @@ -366,9 +367,8 @@ KeepFileRestoredFromArchive(const char *path, const char *xlogfname) if (stat(xlogfpath, &statbuf) == 0) { - char oldpath[MAXPGPATH]; - #ifdef WIN32 + char oldpath[MAXPGPATH]; static unsigned int deletedcounter = 1; /* @@ -390,18 +390,17 @@ KeepFileRestoredFromArchive(const char *path, const char *xlogfname) errmsg("could not rename file \"%s\" to \"%s\": %m", xlogfpath, oldpath))); } -#else - /* same-size buffers, so this never truncates */ - strlcpy(oldpath, xlogfpath, MAXPGPATH); -#endif if (unlink(oldpath) != 0) ereport(FATAL, (errcode_for_file_access(), errmsg("could not remove file \"%s\": %m", xlogfpath))); +#endif reload = true; } + INJECTION_POINT("keepfile-restored-before-rename", NULL); + durable_rename(path, xlogfpath, ERROR); /* diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build index 39ec8c4946d..01343cdff9b 100644 --- a/src/test/recovery/meson.build +++ b/src/test/recovery/meson.build @@ -64,6 +64,7 @@ tests += { 't/053_standby_login_event_trigger.pl', 't/054_unlogged_sequence_promotion.pl', 't/055_cascade_reconnect.pl', + 't/056_archive_restore_walsender_race.pl', ], }, } diff --git a/src/test/recovery/t/056_archive_restore_walsender_race.pl b/src/test/recovery/t/056_archive_restore_walsender_race.pl new file mode 100644 index 00000000000..3b2d0ced9ac --- /dev/null +++ b/src/test/recovery/t/056_archive_restore_walsender_race.pl @@ -0,0 +1,126 @@ +# KeepFileRestoredFromArchive must not unlink before durable_rename +# or a concurrent walsender can see a missing segment + +use strict; +use warnings FATAL => 'all'; +use File::Path qw(rmtree); +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +# Windows already renames aside under FILE_SHARE_DELETE so the Unix race differs +plan skip_all => 'Unix-specific race' + if $PostgreSQL::Test::Utils::windows_os; +plan skip_all => 'injection points required' + unless $ENV{enable_injection_points} eq 'yes'; + +my $inj = 'keepfile-restored-before-rename'; +my $gate = "$PostgreSQL::Test::Utils::tmp_check/g$$"; + +# Switch WAL and wait until the archiver has finished each segment +sub advance_wal_and_wait_archive +{ + my ($node, $num) = @_; + + for (1 .. $num) + { + my $seg = $node->safe_psql('postgres', + 'SELECT pg_walfile_name(pg_current_wal_lsn())'); + $node->safe_psql( + 'postgres', q{ + SELECT pg_logical_emit_message(false, '', 'foo'); + SELECT pg_switch_wal(); + }); + $node->poll_query_until('postgres', + "SELECT '$seg' <= last_archived_wal FROM pg_stat_archiver") + or die "timed out waiting for $seg to be archived"; + } +} + +my $p = PostgreSQL::Test::Cluster->new('p'); +$p->init( + has_archiving => 1, allows_streaming => 1, + extra => ['--wal-segsize', '1']); +$p->append_conf('postgresql.conf', + "shared_preload_libraries = 'injection_points'"); +$p->start; +$p->safe_psql('postgres', 'CREATE EXTENSION injection_points'); +advance_wal_and_wait_archive($p, 2); +$p->backup('b'); +advance_wal_and_wait_archive($p, 3); + +my $a = $p->archive_dir; +# Latest archived segment is forced through KeepFileRestoredFromArchive +my $w = (sort grep { /^[0-9A-F]{24}$/ } slurp_dir($a))[-1]; +my $prev = substr($w, 0, 16) . sprintf('%08X', hex(substr($w, 16, 8)) - 1); +my $segno = hex(substr($w, 16, 8)); +my $w_lsn = sprintf('%X/%08X', ($segno * 1048576) >> 32, + ($segno * 1048576) & 0xFFFFFFFF); + +# Copy the target WAL segment into pg_wal first so restore must replace it +# Gate restore of that segment until the injection point is attached before rename +my $s = PostgreSQL::Test::Cluster->new('s'); +$s->init_from_backup($p, 'b', has_restoring => 1); +system('cp', "$a/$w", $s->data_dir . "/pg_wal/$w") == 0 or die $!; +$s->append_conf('postgresql.conf', qq{ +recovery_prefetch = off +shared_preload_libraries = 'injection_points' +restore_command = 'case "%f" in $w) while [ ! -f $gate ]; do sleep 0.01; done ;; esac; cp "$a/%f" "%p"' +}); +$s->start; +$s->wait_for_log(qr/restored log file "\Q$prev\E" from archive/); +$s->safe_psql('postgres', "SELECT injection_points_attach('$inj','wait')"); +system('touch', $gate); +$s->wait_for_event('startup', $inj); + +# The preplaced segment must still be present at this point before rename +my $w_path = $s->data_dir . "/pg_wal/$w"; +ok(-e $w_path, 'segment still present before rename'); + +# Open a real walsender on the standby while paused in that window +my $stream_dir = PostgreSQL::Test::Utils::tempdir_short(); +my $recv = IPC::Run::start( + [ + 'pg_receivewal', + '--dbname' => $s->connstr('postgres'), + '--directory' => $stream_dir, + '--verbose', + '--no-loop', + ]); +$s->poll_query_until('postgres', + q{SELECT count(*) > 0 FROM pg_stat_replication}) + or die "timed out waiting for walsender on standby"; +pass('walsender started on standby during rename window'); +$recv->signal('TERM'); +$recv->finish; +rmtree($stream_dir); + +# Standby still has the target segment so its walsender cannot see it missing +# Remove the same segment on the primary and START_REPLICATION at its first LSN +# to show the missing segment error a walsender would hit +my $p_w_path = $p->data_dir . "/pg_wal/$w"; +ok(-e $p_w_path, 'primary still has the raced segment'); +unlink($p_w_path) or die "could not unlink primary $w: $!"; + +my $logstart = -s $p->logfile; +$p->psql( + 'postgres', + "START_REPLICATION PHYSICAL $w_lsn", + replication => 1); +$p->wait_for_log( + qr/requested WAL segment \Q$w\E has already been removed/, + $logstart); +pass('walsender errors when the raced segment is missing'); + +# Put the segment back on the primary then wake startup to finish the rename +system('cp', "$a/$w", $p_w_path) == 0 or die $!; +$s->safe_psql('postgres', "SELECT injection_points_wakeup('$inj')"); +$s->poll_query_until( + 'postgres', + qq{SELECT count(*) = 0 FROM pg_stat_activity + WHERE backend_type = 'startup' + AND wait_event = '$inj'}) + or die "timed out waiting for startup to leave $inj"; +ok(-e $w_path, 'restored segment present after rename'); + +done_testing(); -- 2.55.0
