pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedReplicationSlotRelease() clobbers another backend's statusFlags entry
5+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2026-09-30 9510a826e4 453ebae70c c5666f7e16 295f75b4bb 02c32116d5 3d15afe7df 56b306f8b2 Fix clobbering of proc entry's statusFlags during slot invalidation
* ReplicationSlotRelease() clobbers another backend's statusFlags entry
@ 2026-09-28 09:54 Vlad Lesin <vladlesin@gmail.com>
2026-09-29 07:10 ` RE: ReplicationSlotRelease() clobbers another backend's statusFlags entry Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
0 siblings, 1 reply; 5+ messages in thread
From: Vlad Lesin @ 2026-09-28 09:54 UTC (permalink / raw)
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; +Cc: Andrey Borodin <x4mmm@yandex-team.ru>
Hi,
We chased a sporadic crash in our CI for a while and it turned out to be
a live PostgreSQL bug, so here it is with a patch.
A checkpoint that invalidates an obsolete replication slot wipes the
shared status flags of an unrelated backend. On an assert-enabled server
that backend aborts at its next commit and the postmaster takes the
whole cluster down with it. Without assertions nobody notices, and in
the worst case vacuum then removes rows a standby still needs. It
affects every branch from v14 up, and it is not primary-only: a standby
reaches the same code during a restartpoint, and from v16 during
ordinary replay.
The mechanism is a one-line indexing mistake. ReplicationSlotRelease()
ends with:
/* 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);
That is correct only for a process that is in the proc array. An
auxiliary process never enters it, so its pgxactoff is still the zero
InitProcGlobal() left there, and the store lands on the
ProcGlobal->statusFlags[] entry of whichever backend owns offset 0.
The two statements are not the same operation. The bit clear applies to
the caller's own private copy, which in an auxiliary process is zero
anyway; the store is an assignment onto a foreign entry, so the victim
loses every flag it holds, not just PROC_IN_LOGICAL_DECODING.
Auxiliary processes do get here. InvalidatePossiblyObsoleteSlot() takes
the doomed slot by hand, setting MyReplicationSlot and active_pid under
the slot's spinlock, and then lets go of it through
ReplicationSlotRelease():
CreateCheckPoint() checkpointer
CreateRestartPoint() startup process
xlog_redo() startup process, v16+
ResolveRecoveryConflictWithSnapshot() startup process, v16+
-> InvalidateObsoleteReplicationSlots()
-> InvalidatePossiblyObsoleteSlot()
-> ReplicationSlotRelease()
The last two came in with 26669757b6a, which is why a standby can reach
this during replay from v16 on. From v18 idle_replication_slot_timeout
can trigger the invalidation as well, so max_slot_wal_keep_size is no
longer needed to get there at all.
The damage surfaces later, and only sometimes. The victim has to reach
the end of its transaction while running VACUUM or CREATE INDEX
CONCURRENTLY. ProcArrayEndTransaction() then compares its private flags
against ProcGlobal->statusFlags[]. In an --enable-cassert build the
assertion there fires:
TRAP: failed Assert("proc->statusFlags ==
ProcGlobal->statusFlags[proc->pgxactoff]")
LOG: client backend was terminated by signal 6: Aborted
That is how we found it: the primary died while the checkpointer was
invalidating a slot that had fallen behind max_slot_wal_keep_size, and
autovacuum happened to be vacuuming pg_depend at that moment.
The victim is the proc array member with the lowest PGPROC slot number,
normally the longest-connected regular backend, or an autovacuum worker
when no client is connected. Whether the damage is noticed depends on
what that process happens to be doing, which is why the crash looks random.
Without assertions nothing complains, and what the corruption costs
depends on which flag the victim lost. Losing PROC_IN_VACUUM or
PROC_IN_SAFE_IC only makes horizons more conservative. The next commit
rewrites the entry anyway.
PROC_AFFECTS_ALL_HORIZONS is the bad one. InitWalSender() sets it on a
walsender that connected without a database, and nothing sets it again
for the life of that connection. If such a walsender is the victim,
ComputeXidHorizons() stops applying its hot standby feedback xmin to
data_oldest_nonremovable. Vacuum can then remove rows the standby still
needs.
The attached patch adds a reproducer,
src/test/recovery/t/057_slot_invalidation_statusflags.pl. It creates a
physical slot nobody streams from and pushes it past
max_slot_wal_keep_size. A conflicting ShareUpdateExclusiveLock parks a
backend inside VACUUM. vacuum_rel() sets PROC_IN_VACUUM before it opens
the relation, so that backend waits with the flag set. Then CHECKPOINT
invalidates the slot.
I ran the test on REL_13_22, REL_14_24, REL_16_15, REL_17_11, REL_18_6
and master. v13 passes, everything from v14 up fails the same way. v15
and v19 carry the statement verbatim, so the range is v14 through master.
The patch skips the update unless PROC_IN_LOGICAL_DECODING is actually
set. 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.
With the patch applied on master I ran the core regression tests, plus
the TAP suites in src/test/recovery and src/test/subscription. All of
them pass.
Related, neither of them this bug:
https://postgr.es/m/tencent_CA7420C82971BC4B64F0748A7D2898A4720A%40qq.com
reports the same assertion from a different route, an exiting walsender
whose pgxactoff was stale because ProcArrayRemove() had already
compacted the ProcGlobal arrays. That one is v14-only, since 2f6501fa3c5
moved the slot release to before_shmem_exit() in v15, and nothing from
the thread was committed.
1f2e51e3c7c, and its back-branch commits, fixed ReplicationSlotRelease()
for a caller in single user mode. Same shape as this one: the function
assumes its caller is an ordinary backend.
--
Best regards,
Vlad
Attachments:
v1-0001-Fix-clobbering-of-shared-statusFlags-when-a-slot-.patch (text/x-patch, 9.6K) download
From b600a25761f51fad938efa03f286165d215c366a Mon Sep 17 00:00:00 2001
From: Vlad Lesin <vlad_lesin@mail.ru>
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
* RE: ReplicationSlotRelease() clobbers another backend's statusFlags entry
2026-09-28 09:54 ReplicationSlotRelease() clobbers another backend's statusFlags entry Vlad Lesin <vladlesin@gmail.com>
@ 2026-09-29 07:10 ` Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
2026-09-29 16:22 ` Re: ReplicationSlotRelease() clobbers another backend's statusFlags entry Vlad Lesin <vladlesin@gmail.com>
2026-09-30 01:21 ` Re: ReplicationSlotRelease() clobbers another backend's statusFlags entry Michael Paquier <michael@paquier.xyz>
0 siblings, 2 replies; 5+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2026-09-29 07:10 UTC (permalink / raw)
To: Vlad Lesin <vladlesin@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; +Cc: Andrey Borodin <x4mmm@yandex-team.ru>
Hi,
> We chased a sporadic crash in our CI for a while and it turned out to be
> a live PostgreSQL bug, so here it is with a patch.
Cool, good catch!
>
> The mechanism is a one-line indexing mistake. ReplicationSlotRelease()
> ends with:
>
> /* 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);
>
> That is correct only for a process that is in the proc array. An
> auxiliary process never enters it, so its pgxactoff is still the zero
> InitProcGlobal() left there, and the store lands on the
> ProcGlobal->statusFlags[] entry of whichever backend owns offset 0.
Actually InitProcGlobal() does not exist in HEAD, but your point seems correct:
InitProcess(), which is for backends, walsenders, bgworkers, autovac processes,
slotsync worker and bootstrap process, picks MyProc from the ProcGlobal, then
upcoming ProcArrayAdd() sets the index.
However, auxiliary processes are initialized by InitAuxiliaryProcess(), and it picks
MyProc from AuxiliaryProcessMainCommon().
> The patch skips the update unless PROC_IN_LOGICAL_DECODING is actually
> set.
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()?
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 */
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.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: ReplicationSlotRelease() clobbers another backend's statusFlags entry
2026-09-28 09:54 ReplicationSlotRelease() clobbers another backend's statusFlags entry Vlad Lesin <vladlesin@gmail.com>
2026-09-29 07:10 ` RE: ReplicationSlotRelease() clobbers another backend's statusFlags entry Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
@ 2026-09-29 16:22 ` Vlad Lesin <vladlesin@gmail.com>
1 sibling, 0 replies; 5+ messages in thread
From: Vlad Lesin @ 2026-09-29 16:22 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; +Cc: Andrey Borodin <x4mmm@yandex-team.ru>
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
Attachments: download all (2 files, .tar.gz)
v2-0001-Fix-clobbering-of-shared-statusFlags-when-a-slot-.patch (text/x-patch, 7.8K) download
From b9c6b7c9da73fade0512ed768983eff0958411e4 Mon Sep 17 00:00:00 2001
From: Vlad Lesin <vlad_lesin@mail.ru>
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
v2-0002-Assert-that-a-PGPROC-is-in-the-proc-array-before-.patch (text/x-patch, 6.7K) download
From 5e544af8ecdd00cf5cfe4714a16b7188b16b2217 Mon Sep 17 00:00:00 2001
From: Vlad Lesin <vlad_lesin@mail.ru>
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 <kuroda.hayato@fujitsu.com>
Discussion: https://postgr.es/m/889a06dd-45fc-423b-9dd2-87d5b5dc60c6@gmail.com
---
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
* Re: ReplicationSlotRelease() clobbers another backend's statusFlags entry
2026-09-28 09:54 ReplicationSlotRelease() clobbers another backend's statusFlags entry Vlad Lesin <vladlesin@gmail.com>
2026-09-29 07:10 ` RE: ReplicationSlotRelease() clobbers another backend's statusFlags entry Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
@ 2026-09-30 01:21 ` Michael Paquier <michael@paquier.xyz>
2026-09-30 08:34 ` Re: ReplicationSlotRelease() clobbers another backend's statusFlags entry Vlad Lesin <vladlesin@gmail.com>
1 sibling, 1 reply; 5+ messages in thread
From: Michael Paquier @ 2026-09-30 01:21 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; +Cc: Vlad Lesin <vladlesin@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Andrey Borodin <x4mmm@yandex-team.ru>
On Tue, Sep 29, 2026 at 07:10:38AM +0000, Hayato Kuroda (Fujitsu) wrote:
> 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()?
I am not sure that we need that, TBH. This feels like an unnecessary
belt-and-suspenders set of assertions.
> 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.
Proposed code and tests have been AI-generated, hence the prose.
Let's simplify that.
> /* avoid unnecessary dirtying shared cache lines */
>
> 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.
Regarding this part, I am unconvinced that this is worth the cycles
spent on. I am OK to be proved wrong, but for one the test assumes
that we could crash, which is an anti-pattern with the fix in place
because we don't crash once the status flags are not correctly
filtered.
Saying all that, only doing v2-0001 for the slot release seems good
enough here, down to v14. I'd suspect that for some code out there
clearing the flags where we should not is a trap in disguise..
Will process. :)
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: ReplicationSlotRelease() clobbers another backend's statusFlags entry
2026-09-28 09:54 ReplicationSlotRelease() clobbers another backend's statusFlags entry Vlad Lesin <vladlesin@gmail.com>
2026-09-29 07:10 ` RE: ReplicationSlotRelease() clobbers another backend's statusFlags entry Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
2026-09-30 01:21 ` Re: ReplicationSlotRelease() clobbers another backend's statusFlags entry Michael Paquier <michael@paquier.xyz>
@ 2026-09-30 08:34 ` Vlad Lesin <vladlesin@gmail.com>
0 siblings, 0 replies; 5+ messages in thread
From: Vlad Lesin @ 2026-09-30 08:34 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Andrey Borodin <x4mmm@yandex-team.ru>
On 9/30/26 4:21 AM, Michael Paquier wrote:
>> 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.
>
> Regarding this part, I am unconvinced that this is worth the cycles
> spent on. I am OK to be proved wrong, but for one the test assumes
> that we could crash, which is an anti-pattern with the fix in place
> because we don't crash once the status flags are not correctly
> filtered.
Without the fix, the test fails even without the asserts from the
v2-0002 patch.
--
Best regards,
Vlad
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2026-09-30 08:34 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-09-28 09:54 ReplicationSlotRelease() clobbers another backend's statusFlags entry Vlad Lesin <vladlesin@gmail.com>
2026-09-29 07:10 ` Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
2026-09-29 16:22 ` Vlad Lesin <vladlesin@gmail.com>
2026-09-30 01:21 ` Michael Paquier <michael@paquier.xyz>
2026-09-30 08:34 ` Vlad Lesin <vladlesin@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox