From b600a25761f51fad938efa03f286165d215c366a Mon Sep 17 00:00:00 2001 From: Vlad Lesin Date: Wed, 23 Sep 2026 10:33:49 -0700 Subject: [PATCH v1] 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 a slot that has fallen behind max_slot_wal_keep_size and releases it through ReplicationSlotRelease(); the startup process does so during a restartpoint, and 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. Assert as much at both places, adding AmAuxiliaryProcess(); the auxiliary backend types are already contiguous in BackendType. Back-patch to v14, where 5788e258bb2 introduced the array indexed by pgxactoff. Backpatch-through: 14 --- src/backend/replication/logical/logical.c | 3 + src/backend/replication/slot.c | 23 +++- src/include/miscadmin.h | 3 + src/test/recovery/meson.build | 1 + .../t/057_slot_invalidation_statusflags.pl | 129 ++++++++++++++++++ 5 files changed, 154 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/057_slot_invalidation_statusflags.pl diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c index 98e5f1dd8f9..4d8335b4a7d 100644 --- a/src/backend/replication/logical/logical.c +++ b/src/backend/replication/logical/logical.c @@ -272,6 +272,9 @@ StartupDecodingContext(List *output_plugin_options, */ if (!IsTransactionOrTransactionBlock()) { + /* Only a proc array member has a pgxactoff of its own. */ + Assert(!AmAuxiliaryProcess()); + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); MyProc->statusFlags |= PROC_IN_LOGICAL_DECODING; ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags; diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 63ce6d27885..663e44b2123 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -831,11 +831,24 @@ 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); + /* + * This function is also reached from an auxiliary process: + * InvalidatePossiblyObsoleteSlot() acquires the doomed slot on behalf of + * the checkpointer, or of the startup process during a restartpoint, and + * releases it here. An auxiliary process is never entered into the proc + * array, so its pgxactoff is still the zero it was initialized to, and + * the store below would land on the ProcGlobal->statusFlags[] array entry + * of whichever backend owns offset 0. + */ + if (MyProc->statusFlags & PROC_IN_LOGICAL_DECODING) + { + Assert(!AmAuxiliaryProcess()); + + 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/include/miscadmin.h b/src/include/miscadmin.h index 0fc59af02b9..c35c58a604f 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -410,6 +410,9 @@ extern PGDLLIMPORT BackendType MyBackendType; (AmAutoVacuumLauncherProcess() || \ AmLogicalSlotSyncWorkerProcess()) +#define AmAuxiliaryProcess() \ + (MyBackendType >= B_ARCHIVER && MyBackendType <= B_WAL_WRITER) + /* * Backend types that are spawned by the postmaster to serve a client or * replication connection. These backend types have in common that they are diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build index 72113c5ac6e..e8387ca7156 100644 --- a/src/test/recovery/meson.build +++ b/src/test/recovery/meson.build @@ -65,6 +65,7 @@ tests += { 't/054_unlogged_sequence_promotion.pl', 't/055_cascade_reconnect.pl', 't/056_standby_snapshot_export.pl', + 't/057_slot_invalidation_statusflags.pl', ], }, } diff --git a/src/test/recovery/t/057_slot_invalidation_statusflags.pl b/src/test/recovery/t/057_slot_invalidation_statusflags.pl new file mode 100644 index 00000000000..0fcb58761ae --- /dev/null +++ b/src/test/recovery/t/057_slot_invalidation_statusflags.pl @@ -0,0 +1,129 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group +# +# A checkpoint that invalidates an obsolete replication slot must not corrupt +# the shared ProcGlobal->statusFlags array. +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 +# victim backend is parked inside VACUUM. +$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 +log_checkpoints = on + +# 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('doomed', true)"); + +# Leave the slot -- which nobody is streaming from, so the checkpointer gets to +# acquire it itself -- far enough behind that the next checkpoint has to +# invalidate it. +advance_wal($node, 2); + +# The control session runs over a walsender connection on purpose: its PGPROC +# comes from the walsender free list, so it cannot take the dense-array slot +# that the vacuuming 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 ShareUpdateExclusiveLock parks the victim in exactly the state +# the assertion at commit checks. +$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 for it through the control session. An ordinary poll_query_until() +# would connect a second regular backend, and that one -- not the vacuum -- +# could end up owning dense slot 0. +my $blocked = ''; +foreach my $i (1 .. 300) +{ + $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 parked 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 = 'doomed'}), + '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 = 'doomed'}), + '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'); + +# A server that has just aborted takes this session down with it, so the query +# has to be allowed to fail. +my $vacuumed = eval { $vac->query_safe('SELECT 1') }; + +# Older branches spell this "TRAP: FailedAssertion(", newer ones +# "TRAP: failed Assert(". +ok(!$node->log_contains(qr/TRAP: fail/i, $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 parked VACUUM committed and its session survived'); + +# Both sessions are gone if the server went down under them, so neither may be +# allowed to take the test with it. +eval { $vac->quit; }; +eval { $ctl->quit; }; + +done_testing(); + +# Burn through $n WAL segments. +sub advance_wal +{ + my ($node, $n) = @_; + + for (my $i = 0; $i < $n; $i++) + { + $node->safe_psql('postgres', + 'CREATE TABLE t (); DROP TABLE t; SELECT pg_switch_wal();'); + } + return; +} base-commit: 374522aa63af1187c15dd21db0b2c988b55c9a3a -- 2.52.0