ReplicationSlotRelease() clobbers another backend's statusFlags entry
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253966psql -h localhost -U postgresBuilt from patchset v3 (message #3), September 29, 2026 at 04:36 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253966_3 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253966_3 && git checkout t253966_3Patchset v3 (message #3) is on t253966_3
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:
/messages/by-id/tencent_CA7420C82971BC4B64F0748A7D2898A4720A@qq.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
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
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:
t253966_3v2-0001-Fix-clobbering-of-shared-statusFlags-when-a-slot-.patchtext/x-patch; charset=UTF-8; name=v2-0001-Fix-clobbering-of-shared-statusFlags-when-a-slot-.patchDownload+130-6
v2-0002-Assert-that-a-PGPROC-is-in-the-proc-array-before-.patchtext/x-patch; charset=UTF-8; name=v2-0002-Assert-that-a-PGPROC-is-in-the-proc-array-before-.patchDownload+25-5
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
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