Hi Greg,
On Sun, Oct 25, 2026 at 7:12 PM Greg Burd <[email protected]> wrote:
> - The FILEOPS WRITE inverse treated a failed stat() of its file like a
> missing file and reported success, so a crash loser's overwritten
> bytes were never restored.
I tested this with a real EACCES instead of the injection point. A
crash loser overwrites the first 10 bytes of a file, and before the
restart the file's directory loses its search permission (the server
does not run as root).
v7: no ATM entry, the bytes stay overwritten (3/3 runs).
v8: the loser goes to the ATM, and once the directory is readable
again the worker restores the bytes (3/3).
That only holds while the error is short. If the directory is still
unreadable when the revert worker starts (I held it for 10 s), the
worker walks the chain 3 times, about 1 s and 2 s apart, logs
"giving up on 1 UNDO record(s) of transaction N that failed to apply
3 times" and forgets the entry. The bytes stay overwritten, as in v7
(3/3). So at the default logical_revert_naptime, an EACCES or EIO
that lasts more than about 3 s ends the way it did before, now with
a WARNING. Is that intended for a record that failed to apply? From
reading process_revert_entry(), LOGICAL_REVERT_MAX_RECORD_RETRIES
gives up on it the same way as on a malformed chain.
> - The logical revert launcher gave up on a database whose worker could
> not be registered (no free background-worker slot): it recorded the
> database as served anyway, so its ATM entries were never reverted and
> their WAL was kept for good.
I tested this with real slot exhaustion. worker_spi workers take
every free slot, an ATM entry is queued the way test_fileops/019
does it, and the launcher is woken and fails to register. Then the
worker_spi workers are terminated and the launcher is woken again.
v7: the entry is not drained after 90 s (past one 60 s scan), no
revert worker runs, and its WAL segment is still kept after 4
segment switches and a CHECKPOINT (3/3).
v8: the launcher retries and the entry is drained (3/3).
> - When the revert worker's 64-slot retry table was full, a chain that
> failed to apply was given up on its first failure, marked reverted
> and forgotten.
Your test_idxundo/020 leaves 64 of 65 entries after one pass on v7
and fails (3/3). On v8 it leaves 65 of 65 and passes (3/3).
Each check also fails on v8 with only its fix reverted (3/3 each):
my two tests, your test_fileops/020 and your test_idxundo/020.
One thing for the 0001/0002/0005 post: the tests for the launcher fix
and the retry-table fix (test_idxundo/021 and 020) are in 0006, and
they need idxundo_atm_add() or an index_undo table. Without 0006
those two fixes have no test. The attached nocfbot diff adds my two
tests to test_fileops as t/021 and t/022. They only need
test_fileops, injection_points and worker_spi, so they apply on top
of 0005. I have not ported test_idxundo/020. t/021 takes HOLD=<s> in
the environment (default 0) for the 10 s case, which fails on v8.
On master dfd7ede529f all 21 patches apply. 0001-0005 together
build with no warnings (cassert), and these pass: make check (239),
test_fileops (19 files, including 019, 020 and the two new ones),
and recovery 061-064, 071, 072 and 090.
Limits: cassert only, Linux, one machine, no timings. v7 here is the
full v7 series. fileops_undo.c and logical_revert_worker.c are the
same in v7-0005, except for one whitespace line.
Regards,
Manu
diff --git a/src/test/modules/test_fileops/Makefile
b/src/test/modules/test_fileops/Makefile
index 69100c7527..4da4436572 100644
--- a/src/test/modules/test_fileops/Makefile
+++ b/src/test/modules/test_fileops/Makefile
@@ -5,7 +5,7 @@ DATA = test_fileops--1.0.sql
PGFILEDESC = "test_fileops - test transactional file operations"
REGRESS = test_fileops
TAP_TESTS = 1
-EXTRA_INSTALL = src/test/modules/injection_points
+EXTRA_INSTALL = src/test/modules/injection_points src/test/modules/worker_spi
export enable_injection_points
ifdef USE_PGXS
PG_CONFIG = pg_config
diff --git a/src/test/modules/test_fileops/meson.build
b/src/test/modules/test_fileops/meson.build
index a31b9866a8..3054f2166a 100644
--- a/src/test/modules/test_fileops/meson.build
+++ b/src/test/modules/test_fileops/meson.build
@@ -52,6 +52,8 @@ tests += {
't/018_fileops_rename.pl',
't/019_fileops_logical_revert_fsync_batch.pl',
't/020_fileops_write_stat_error.pl',
+ 't/021_fileops_write_stat_eacces.pl',
+ 't/022_fileops_revert_register_fail.pl',
],
},
}
diff --git a/src/test/modules/test_fileops/t/021_fileops_write_stat_eacces.pl
b/src/test/modules/test_fileops/t/021_fileops_write_stat_eacces.pl
new file mode 100644
index 0000000000..37bdc28b4b
--- /dev/null
+++ b/src/test/modules/test_fileops/t/021_fileops_write_stat_eacces.pl
@@ -0,0 +1,100 @@
+# Copyright (c) 2026, PostgreSQL Global Development Group
+
+# FILEOPS WRITE inverse vs. a real lstat() error (no injection point).
+#
+# A crash loser overwrote the first bytes of a file through FILEOPS WRITE.
+# Before the restart, the search permission of the file's directory is
+# removed, so the startup undo phase's lstat() fails with EACCES (the file is
+# still there). The overwritten bytes must not be dropped: the loser must be
+# deferred to the ATM and the bytes restored once the directory is readable.
+#
+# HOLD (seconds, env, default 0) is how long the EACCES lasts after the
+# logical revert worker is allowed to run. HOLD=0 lifts it before the worker
+# starts. A HOLD longer than the worker's retry budget shows what happens
+# when the stat error outlives it.
+#
+# Must not run as root (root ignores the directory permission).
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+use Time::HiRes qw(usleep);
+
+plan skip_all => 'running as root: EACCES cannot be produced' if $> == 0;
+
+my $hold = $ENV{HOLD} // 0;
+
+my $node = PostgreSQL::Test::Cluster->new('fileops_write_stat_eacces');
+$node->init;
+$node->append_conf(
+ 'postgresql.conf', q{
+wal_level = replica
+autovacuum = off
+max_logical_revert_workers = 0
+});
+$node->start;
+$node->safe_psql('postgres', 'CREATE EXTENSION test_fileops');
+
+my $dir = PostgreSQL::Test::Utils::tempdir('eacces');
+my $path = "$dir/w.dat";
+open(my $fh, '>', $path) or die "open $path: $!";
+print $fh 'X' x 1024;
+close($fh);
+
+# A WRITE in flight when the server crashes: a crash loser.
+my $bg = $node->background_psql('postgres', on_error_stop => 0);
+$bg->query_safe('BEGIN');
+$bg->query_safe("SELECT test_fileops_write('$path', 0, 'ROLLBACKME')");
+my $xid = $bg->query_safe('SELECT pg_current_xact_id()');
+$node->safe_psql('postgres', 'SELECT pg_switch_wal()');
+my $offset = -s $node->logfile;
+$node->stop('immediate');
+$bg->{run}->finish;
+
+is(substr(slurp_file($path), 0, 10), 'ROLLBACKME', 'loser bytes on disk');
+
+# Real EACCES for the startup undo phase: no search permission on the dir.
+chmod(0600, $dir) or die "chmod $dir: $!";
+$node->start;
+
+my $log = slurp_file($node->logfile, $offset);
+like($log, qr/UNDO recovery: rolling back transaction $xid\b/,
+ 'the WRITE transaction was found as a loser');
+my $stat_warn = () = $log =~ /FILEOPS UNDO WRITE: could not stat/g;
+cmp_ok($stat_warn, '>=', 1, 'startup lstat() failed for real');
+my $atm = $node->safe_psql('postgres',
+ 'SELECT test_fileops_atm_oldest_lsn() IS NOT NULL');
+note "RESULT after_startup atm_entry=$atm stat_warnings=$stat_warn";
+is($atm, 't', 'the stat error deferred the loser to the ATM');
+
+# Let the worker run; keep the EACCES for HOLD seconds.
+$offset = -s $node->logfile;
+$node->append_conf('postgresql.conf', 'max_logical_revert_workers = 2');
+if ($hold > 0)
+{
+ $node->restart;
+ sleep($hold);
+ chmod(0700, $dir) or die "chmod $dir: $!";
+}
+else
+{
+ chmod(0700, $dir) or die "chmod $dir: $!";
+ $node->restart;
+}
+
+my $drained = $node->poll_query_until('postgres',
+ 'SELECT test_fileops_atm_oldest_lsn() IS NULL');
+# Give the worker a few more passes so a late restore is not missed.
+sleep(5);
+$log = slurp_file($node->logfile, $offset);
+my $retries = () = $log =~ /failed to apply, will retry/g;
+my $gaveup = () = $log =~ /logical revert: giving up on/g;
+my $bytes = substr(slurp_file($path), 0, 10);
+note "RESULT hold=${hold}s atm_drained=" . ($drained ? 1 : 0)
+ . " retries=$retries gave_up=$gaveup first_bytes=$bytes";
+ok($drained, 'the ATM entry was drained');
+is($bytes, 'XXXXXXXXXX', 'the overwritten bytes were restored');
+
+$node->stop;
+done_testing();
diff --git
a/src/test/modules/test_fileops/t/022_fileops_revert_register_fail.pl
b/src/test/modules/test_fileops/t/022_fileops_revert_register_fail.pl
new file mode 100644
index 0000000000..b75c4014ac
--- /dev/null
+++ b/src/test/modules/test_fileops/t/022_fileops_revert_register_fail.pl
@@ -0,0 +1,123 @@
+# Copyright (c) 2026, PostgreSQL Global Development Group
+
+# Logical revert launcher vs. real background-worker slot exhaustion (no
+# injection point on the registration path).
+#
+# Every free bgworker slot is taken by worker_spi workers, an ATM entry is
+# queued, and the launcher is woken: RegisterDynamicBackgroundWorker() fails
+# for real. The worker_spi workers are then terminated. A correct launcher
+# retries the database on a later scan and the entry is drained; one that
+# recorded the database as served never does, and the entry pins its WAL.
+#
+# The ATM entry is made as in test_fileops/019: the inline rollback fails
+# after its batch applies (injection point undo-apply-after-batch), so the
+# abort WAL owns the inverses and the chain is handed to the ATM.
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+ plan skip_all => 'Injection points not supported by this build';
+}
+
+my $node = PostgreSQL::Test::Cluster->new('fileops_revert_register_fail');
+$node->init;
+$node->append_conf(
+ 'postgresql.conf', q{
+wal_level = replica
+autovacuum = off
+max_worker_processes = 8
+max_logical_revert_workers = 2
+});
+$node->start;
+$node->safe_psql('postgres',
+ 'CREATE EXTENSION injection_points; CREATE EXTENSION test_fileops;'
+ . ' CREATE EXTENSION worker_spi');
+
+# Take every free slot.
+my @spi;
+for my $i (1 .. 64)
+{
+ my $pid = $node->safe_psql('postgres', "SELECT worker_spi_launch($i)");
+ last if $pid eq '';
+ push @spi, $pid;
+}
+my $nspi = scalar @spi;
+note "RESULT slots_taken_by_worker_spi=$nspi";
+cmp_ok($nspi, '>=', 1, 'took the free bgworker slots');
+
+# Queue an ATM entry for this database.
+my $path = $node->safe_psql('postgres',
+ "SELECT test_fileops_create_tempfile('rf.dat')");
+my $bg = $node->background_psql('postgres', on_error_stop => 0);
+$bg->query_safe('SELECT injection_points_set_local()');
+$bg->query_safe(
+ "SELECT injection_points_attach('undo-apply-after-batch', 'error')");
+$bg->query_safe('BEGIN');
+$bg->query_safe("SELECT test_fileops_chmod('rf.dat', 384)");
+$bg->query('ROLLBACK');
+$bg->quit;
+is( $node->safe_psql(
+ 'postgres', 'SELECT test_fileops_atm_oldest_lsn() IS NOT NULL'),
+ 't',
+ 'an ATM entry is queued');
+
+# Wake the launcher: registration fails for real.
+my $offset = -s $node->logfile;
+$node->reload;
+$node->wait_for_log(
+ qr/could not register logical revert worker for database/, $offset);
+pass('launcher failed to register a worker (slots exhausted)');
+
+# Free the slots and wake the launcher again (twice, and wait past one
+# natural 60 s scan).
+my $pids = join(',', @spi);
+$node->safe_psql('postgres',
+ "SELECT pg_terminate_backend(pid) FROM unnest('{$pids}'::int[]) pid");
+$node->poll_query_until('postgres',
+ "SELECT count(*) = 0 FROM pg_stat_activity WHERE pid = ANY('{$pids}')")
+ or die 'worker_spi workers did not exit';
+$node->reload;
+sleep(2);
+$node->reload;
+
+my $drained = 0;
+for (1 .. 90)
+{
+ if ($node->safe_psql('postgres',
+ 'SELECT test_fileops_atm_oldest_lsn() IS NULL') eq 't')
+ {
+ $drained = 1;
+ last;
+ }
+ sleep(1);
+}
+my $workers = $node->safe_psql('postgres',
+ "SELECT count(*) FROM pg_stat_activity"
+ . " WHERE backend_type = 'logical revert worker'");
+
+# WAL retention: is the segment of the oldest unreverted LSN still kept
+# after a few segment switches and a checkpoint?
+my $seg_kept = 'n/a';
+if (!$drained)
+{
+ my $seg = $node->safe_psql('postgres',
+ 'SELECT pg_walfile_name(test_fileops_atm_oldest_lsn())');
+ for (1 .. 4)
+ {
+ $node->safe_psql('postgres',
+ 'SELECT test_fileops_chmod(\'rf.dat\', 420); SELECT
pg_switch_wal()');
+ }
+ $node->safe_psql('postgres', 'CHECKPOINT');
+ $seg_kept = $node->safe_psql('postgres',
+ "SELECT count(*) FROM pg_ls_waldir() WHERE name = '$seg'");
+}
+note "RESULT atm_drained=$drained revert_workers_now=$workers "
+ . "oldest_segment_kept=$seg_kept";
+ok($drained, 'the launcher retried the database and the entry was drained');
+
+$node->stop;
+done_testing();
