ReplicationSlotRelease() clobbers another backend's statusFlags entry - Mailing list pgsql-hackers

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