From b9c6b7c9da73fade0512ed768983eff0958411e4 Mon Sep 17 00:00:00 2001 From: Vlad Lesin Date: Wed, 23 Sep 2026 10:33:49 -0700 Subject: [PATCH v2 1/2] Fix clobbering of shared statusFlags when a slot is invalidated ReplicationSlotRelease() ended with: MyProc->statusFlags &= ~PROC_IN_LOGICAL_DECODING; ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags; That is only correct for a process in the proc array. Auxiliary processes never join it, so their pgxactoff is still the zero it was initialized to, and the store overwrites the entry of whichever backend owns offset 0. The checkpointer and the startup process both get there. InvalidatePossiblyObsoleteSlot() acquires the slot it invalidates and releases it through ReplicationSlotRelease(). The checkpointer does so during checkpoints and restartpoints, for example when a slot has fallen behind max_slot_wal_keep_size; the startup process does so when replay invalidates a slot that conflicts with recovery. The two statements act on different processes. The bit clear applies to the caller's own private copy, which in an auxiliary process is zero already; the store is an assignment rather than a bit clear, and it lands on another backend's entry. The backend at offset 0 therefore loses not just PROC_IN_LOGICAL_DECODING but every flag it holds. Skip the update unless PROC_IN_LOGICAL_DECODING is set. Nothing but StartupDecodingContext() sets that flag, and no auxiliary process reaches it, so nothing changes for a process in the proc array. Back-patch to v14, where 5788e258bb2 introduced the array indexed by pgxactoff. Backpatch-through: 14 --- src/backend/replication/slot.c | 18 ++- src/test/recovery/meson.build | 1 + .../t/058_slot_invalidation_statusflags.pl | 116 ++++++++++++++++++ 3 files changed, 130 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/058_slot_invalidation_statusflags.pl diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 63ce6d27885..c84d021aeaf 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -831,11 +831,19 @@ ReplicationSlotRelease(void) MyReplicationSlot = NULL; } - /* might not have been set when we've been a plain slot */ - LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); - MyProc->statusFlags &= ~PROC_IN_LOGICAL_DECODING; - ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags; - LWLockRelease(ProcArrayLock); + /* + * Only touch ProcGlobal->statusFlags[] if we set + * PROC_IN_LOGICAL_DECODING. An auxiliary process that invalidates a slot + * gets here too. It is not in the proc array, so the entry at its + * pgxactoff is not its own. + */ + if (MyProc->statusFlags & PROC_IN_LOGICAL_DECODING) + { + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + MyProc->statusFlags &= ~PROC_IN_LOGICAL_DECODING; + ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags; + LWLockRelease(ProcArrayLock); + } if (am_walsender) { diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build index ebb12dd8766..089dd5a4348 100644 --- a/src/test/recovery/meson.build +++ b/src/test/recovery/meson.build @@ -66,6 +66,7 @@ tests += { 't/055_cascade_reconnect.pl', 't/056_standby_snapshot_export.pl', 't/057_snapshot_commit_race.pl', + 't/058_slot_invalidation_statusflags.pl', ], }, } diff --git a/src/test/recovery/t/058_slot_invalidation_statusflags.pl b/src/test/recovery/t/058_slot_invalidation_statusflags.pl new file mode 100644 index 00000000000..1fc2a7e70a9 --- /dev/null +++ b/src/test/recovery/t/058_slot_invalidation_statusflags.pl @@ -0,0 +1,116 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group +# +# A checkpoint that invalidates an obsolete replication slot must not corrupt +# the shared ProcGlobal->statusFlags array. +# +# The corruption is only noticed in builds with assertions enabled, by the +# assertion in ProcArrayEndTransaction() that compares a backend's flags with +# its entry in ProcGlobal->statusFlags[]. +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; +use Time::HiRes qw(usleep); + +my $node = PostgreSQL::Test::Cluster->new('primary'); +$node->init(allows_streaming => 1, extra => ['--wal-segsize=1']); + +# No checkpoint may happen on its own: the single CHECKPOINT this test issues +# has to be the one that invalidates the slot, and it has to run while the +# VACUUM below is waiting for its lock. +$node->append_conf( + 'postgresql.conf', qq( +autovacuum = off +checkpoint_timeout = 1h +min_wal_size = 2MB +max_wal_size = 1GB +max_slot_wal_keep_size = 1MB + +# Cluster::init() turns this off, but a server that stays down would make +# teardown bail out before the failure is reported. Let it come back instead, +# and report the crash from the log. +restart_after_crash = on +)); +$node->start; + +$node->safe_psql('postgres', + 'CREATE TABLE vactbl AS SELECT generate_series(1, 1000) AS i'); +$node->safe_psql('postgres', + "SELECT pg_create_physical_replication_slot('lagging', true)"); + +# The slot is not in use, so the checkpointer acquires it itself. Leave it +# far enough behind that the next checkpoint has to invalidate it. +$node->advance_wal(2); + +# The control session runs over a walsender connection on purpose: its PGPROC +# comes from the walsender free list, so it cannot take offset 0 in the proc +# array, which the VACUUM backend below has to own. +my $ctl = $node->background_psql('postgres', replication => 'database'); + +# vacuum_rel() sets PROC_IN_VACUUM before it opens the relation, so a +# conflicting lock makes VACUUM wait with the flag set. At commit, +# ProcArrayEndTransaction() asserts that the flag in ProcGlobal->statusFlags[] +# still matches. +$ctl->query_safe('BEGIN'); +$ctl->query_safe('LOCK TABLE vactbl IN SHARE UPDATE EXCLUSIVE MODE'); + +my $vac = $node->background_psql('postgres', on_error_stop => 0); +$vac->query_until(qr//, "VACUUM vactbl;\n"); + +# Wait through the control session. poll_query_until() would connect another +# regular backend, which could take offset 0 in the proc array instead of the +# VACUUM backend. +my $blocked = ''; +foreach my $i (1 .. 10 * $PostgreSQL::Test::Utils::timeout_default) +{ + $blocked = $ctl->query_safe( + q{SELECT count(*) FROM pg_locks + WHERE relation = 'vactbl'::regclass AND NOT granted}); + last if $blocked eq '1'; + usleep(100_000); +} +is($blocked, '1', 'VACUUM is waiting for the table lock'); +is( $ctl->query_safe( + q{SELECT count(*) FROM pg_stat_activity + WHERE backend_type = 'client backend'}), + '1', + 'the vacuuming backend is the only regular backend connected'); + +isnt( + $ctl->query_safe( + q{SELECT wal_status FROM pg_replication_slots + WHERE slot_name = 'lagging'}), + 'lost', + 'slot has not been invalidated yet'); + +my $log_offset = -s $node->logfile; + +$ctl->query_safe('CHECKPOINT'); + +is( $ctl->query_safe( + q{SELECT wal_status FROM pg_replication_slots + WHERE slot_name = 'lagging'}), + 'lost', + 'checkpoint invalidated the obsolete slot'); + +# Release the lock. The VACUUM now runs to completion and commits, which is +# where the clobbered flags are noticed. +$ctl->query_safe('COMMIT'); + +# If the server crashed, this session is gone too, so the query may fail. +my $vacuumed = eval { $vac->query_safe('SELECT 1') }; + +ok(!$node->log_contains(qr/TRAP: failed Assert/, $log_offset), + 'no assertion failure while invalidating the slot'); +ok(!$node->log_contains(qr/was terminated by signal/, $log_offset), + 'no backend was killed while invalidating the slot'); +is($vacuumed, '1', 'the VACUUM committed and its session survived'); + +# If the server crashed, both sessions are already gone, so quit them inside +# eval. +eval { $vac->quit; }; +eval { $ctl->quit; }; + +done_testing(); base-commit: dca6a9e320e0272f0ea8d7e076cda04b06050866 -- 2.52.0