On 9/29/26 10:10 AM, Hayato Kuroda (Fujitsu) wrote:
ProcArrayEndTransaction() has the similar checking, so seems reasonable.
Only StartupDecodingContext() sets that flag and no auxiliary
process reaches it, so nothing changes for a process in the proc array,
while the checkpointer no longer executes the store at all.
I like the part you added Assert() in StartupDecodingContext(). This can be
worked on separately: can anyone which updates ProcGlobal->statusFlags have
the same Assert()?
For the writers that update their own entry, yes. Besides the two in the
patch (StartupDecodingContext() and ReplicationSlotRelease()), there are
four: vacuum_rel(), set_indexsafe_procflags(), InitWalSender() and
ProcArrayInstallRestoredXmin(). They are reached only from regular
backends, autovacuum workers, walsenders and background workers, all of
which went through InitProcess() and ProcArrayAdd(), so
Assert(!AmAuxiliaryProcess()) holds there. ReplicationSlotRelease() was
the only such path an auxiliary process could take. But for the
procarray.c functions that take a PGPROC argument, the process doing the
store is not always the one whose entry is written, so
Assert(!AmAuxiliaryProcess()) checks the wrong process.
What every writer actually relies on is that the PGPROC occupies the
entry its pgxactoff points to:
proc->pgxactoff >= 0 && proc->pgxactoff < procArray->numProcs &&
procArray->pgprocnos[proc->pgxactoff] == GetNumberFromPGProc(proc)
ProcArrayRemove() comes close with:
myoff = proc->pgxactoff;
Assert(myoff >= 0 && myoff < arrayP->numProcs);
Assert(ProcGlobal->allProcs[arrayP->pgprocnos[myoff]].pgxactoff ==
myoff);
That's why I changed the assertion and added it in the corresponding
places in a separate commit.
Regarding the code, the code comment in ReplicationSlotRelease() may be too
detail.
Can we have something like below? Or adding the possibility that auxiliary
processes
can reach here.
/* avoid unnecessary dirtying shared cache lines */
Fixed.
Regarding the test, I only used to reproduce the issue but not reviewed well,
because not sure it's aimed to be included. It may need more polish, i.e.,
advance_wal() has already been defined.
Yes, I'd like the test to be committed with the fix. Thanks for pointing
out advance_wal(). In v2 the test uses $node->advance_wal() instead of
its ow helper. That method only exists in v17 and later, so the
back-branch versions of the test would still need a local helper. I also
cleaned up the rest of the test a bit.
--
Best regards,
Vlad
From b9c6b7c9da73fade0512ed768983eff0958411e4 Mon Sep 17 00:00:00 2001
From: Vlad Lesin <[email protected]>
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
From 5e544af8ecdd00cf5cfe4714a16b7188b16b2217 Mon Sep 17 00:00:00 2001
From: Vlad Lesin <[email protected]>
Date: Tue, 29 Sep 2026 07:59:43 -0700
Subject: [PATCH v2 2/2] Assert that a PGPROC is in the proc array before
writing its entry
The dense arrays in ProcGlobal are indexed by PGPROC->pgxactoff, which
is only meaningful while the PGPROC is in the proc array. An auxiliary
process never enters it, so its pgxactoff stays zero, and a PGPROC that
has been removed keeps the offset it had, which by then belongs to
another PGPROC or is out of range. A store through such an offset
silently overwrites another process's entry, as the previous commit
fixed for ReplicationSlotRelease().
Add ProcArrayHasProc(), which checks that the PGPROC really occupies
the entry its pgxactoff points to, and assert it wherever a process
changes the flags in its own entry of ProcGlobal->statusFlags[].
The other writers of the array do not need it:
- ProcArrayAdd() is what puts the PGPROC into the array.
- ProcArrayRemove() is only called for a PGPROC that is in the array:
by RemoveProcFromArray(), registered right after ProcArrayAdd(), and
by FinishPreparedTransaction(), which only takes a prepared
transaction whose PGPROC MarkAsPrepared() has added.
- ProcArrayEndTransactionInternal() already asserts that the entry
holds the PGPROC's XID, and a running transaction's XID identifies
its entry.
- ProcArrayEndTransaction() writes only to clear PROC_VACUUM_STATE_MASK
flags. Those are set only where the new assertion is made, and a
process does not leave the proc array before its transaction ends,
since ShutdownPostgres() aborts it first.
Checking that the caller is not an auxiliary process would cover only
half of the problem: a regular backend that has left the proc array
has a stale pgxactoff too.
Suggested-by: Hayato Kuroda <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/commands/indexcmds.c | 1 +
src/backend/commands/vacuum.c | 1 +
src/backend/replication/logical/logical.c | 1 +
src/backend/replication/slot.c | 1 +
src/backend/replication/walsender.c | 1 +
src/backend/storage/ipc/procarray.c | 19 +++++++++++++++++++
src/include/storage/procarray.h | 1 +
7 files changed, 25 insertions(+)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 5a0312fe772..3843c4d3013 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -4810,6 +4810,7 @@ set_indexsafe_procflags(void)
MyProc->xmin == InvalidTransactionId);
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ Assert(ProcArrayHasProc(MyProc));
MyProc->statusFlags |= PROC_IN_SAFE_IC;
ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
LWLockRelease(ProcArrayLock);
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index d8c2f33c615..ee7bdf4698d 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -2078,6 +2078,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams params,
* xmin doesn't become visible ahead of setting the flag.)
*/
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ Assert(ProcArrayHasProc(MyProc));
MyProc->statusFlags |= PROC_IN_VACUUM;
if (params.is_wraparound)
MyProc->statusFlags |= PROC_VACUUM_FOR_WRAPAROUND;
diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
index 98e5f1dd8f9..abd36b98d5b 100644
--- a/src/backend/replication/logical/logical.c
+++ b/src/backend/replication/logical/logical.c
@@ -273,6 +273,7 @@ StartupDecodingContext(List *output_plugin_options,
if (!IsTransactionOrTransactionBlock())
{
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ Assert(ProcArrayHasProc(MyProc));
MyProc->statusFlags |= PROC_IN_LOGICAL_DECODING;
ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
LWLockRelease(ProcArrayLock);
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index c84d021aeaf..dbc9aba82a5 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -840,6 +840,7 @@ ReplicationSlotRelease(void)
if (MyProc->statusFlags & PROC_IN_LOGICAL_DECODING)
{
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ Assert(ProcArrayHasProc(MyProc));
MyProc->statusFlags &= ~PROC_IN_LOGICAL_DECODING;
ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
LWLockRelease(ProcArrayLock);
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index e9331de3df5..6c4d5359e6b 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -357,6 +357,7 @@ InitWalSender(void)
{
Assert(MyProc->xmin == InvalidTransactionId);
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ Assert(ProcArrayHasProc(MyProc));
MyProc->statusFlags |= PROC_AFFECTS_ALL_HORIZONS;
ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
LWLockRelease(ProcArrayLock);
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index b7e03134ed8..1d4b25f85c4 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -645,6 +645,24 @@ ProcArrayRemove(PGPROC *proc, TransactionId latestXid)
LWLockRelease(ProcArrayLock);
}
+/*
+ * ProcArrayHasProc -- is proc in the proc array?
+ *
+ * Returns true if proc occupies the proc array entry its pgxactoff points
+ * to.
+ */
+bool
+ProcArrayHasProc(PGPROC *proc)
+{
+ int pgxactoff = proc->pgxactoff;
+
+ Assert(LWLockHeldByMe(ProcArrayLock));
+
+ return pgxactoff >= 0 &&
+ pgxactoff < procArray->numProcs &&
+ procArray->pgprocnos[pgxactoff] == GetNumberFromPGProc(proc);
+}
+
/*
* ProcArrayEndTransaction -- mark a transaction as no longer running
@@ -2593,6 +2611,7 @@ ProcArrayInstallRestoredXmin(TransactionId xmin, PGPROC *proc)
* Install xmin and propagate the statusFlags that affect how the
* value is interpreted by vacuum.
*/
+ Assert(ProcArrayHasProc(MyProc));
MyProc->xmin = TransactionXmin = xmin;
MyProc->statusFlags = (MyProc->statusFlags & ~PROC_XMIN_FLAGS) |
(proc->statusFlags & PROC_XMIN_FLAGS);
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h
index d718a5b542f..1e0d59b1e62 100644
--- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -21,6 +21,7 @@
extern void ProcArrayAdd(PGPROC *proc);
extern void ProcArrayRemove(PGPROC *proc, TransactionId latestXid);
+extern bool ProcArrayHasProc(PGPROC *proc);
extern void ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid);
extern void ProcArrayClearTransaction(PGPROC *proc);
--
2.52.0
