Hi, On 2024-01-22 15:18:35 +0800, Andy Fan wrote: > I used sigismember(&BlockSig, SIGQUIT) to detect if a process is doing a > quickdie, however this is bad not only because it doesn't work on > Windows, but also it has too poor performance even it impacts on > USE_ASSERT_CHECKING build only. In v8, I introduced a new global > variable quickDieInProgress to handle this.
For reasons you already noted, using sigismember() isn't viable. But I am not convinced by quickDieInProgress either. I think we could just reset the state for the spinlock check in the code handling PANIC, perhaps via a helper function in spin.c. > +void > +VerifyNoSpinLocksHeld(void) > +{ > +#ifdef USE_ASSERT_CHECKING > + if (last_spin_lock_file != NULL) > + elog(PANIC, "A spin lock has been held at %s:%d", > + last_spin_lock_file, last_spin_lock_lineno); > +#endif > +} I think the #ifdef for this needs to be in the header, not here. Otherwise we add a pointless external function call to a bunch of performance critical code. > From f09518df76572adca85cba5008ea0cae5074603a Mon Sep 17 00:00:00 2001 > From: "yizhi.fzh" <yizhi....@alibaba-inc.com> > Date: Fri, 19 Jan 2024 13:57:46 +0800 > Subject: [PATCH v8 2/3] Treat (un)LockBufHdr as a SpinLock. > > The LockBufHdr also used init_local_spin_delay / perform_spin_delay > infrastructure and so it is also possible that PANIC the system > when it can't be acquired in a short time, and its code is pretty > similar with s_lock. so treat it same as SPIN lock when regarding to > misuse of spinlock detection. > --- > src/backend/storage/buffer/bufmgr.c | 1 + > src/include/storage/buf_internals.h | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/src/backend/storage/buffer/bufmgr.c > b/src/backend/storage/buffer/bufmgr.c > index 7d601bef6d..c600a113cf 100644 > --- a/src/backend/storage/buffer/bufmgr.c > +++ b/src/backend/storage/buffer/bufmgr.c > @@ -5409,6 +5409,7 @@ LockBufHdr(BufferDesc *desc) > > init_local_spin_delay(&delayStatus); > > + START_SPIN_LOCK(); > while (true) > { > /* set BM_LOCKED flag */ Seems pretty odd that we now need init_local_spin_delay() and START_SPIN_LOCK(). Note that init_local_spin_delay() also wraps handling of __FILE__, __LINE__ etc, so it seems we're duplicating state here. Greetings, Andres Freund