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
