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
