ReplicationSlotRelease() clobbers another backend's statusFlags entry

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