Re: the s_lock_stuck on perform_spin_delay

Hi,

On 2024-01-04 12:04:07 -0500, Robert Haas wrote:
> On Thu, Jan 4, 2024 at 11:33 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > I believe it's (a). No matter what the reason for a stuck spinlock
> > is, the only reliable method of getting out of the problem is to
> > blow things up and start over. The patch proposed at the top of this
> > thread would leave the system unable to recover on its own, with the
> > only recourse being for the DBA to manually force a crash/restart ...
> > once she figured out that that was necessary, which might take a long
> > while if the only external evidence is an occasional WARNING that
> > might not even be making it to the postmaster log. How's that better?
>
> It's a fair question. I think you're correct if we assume that
> everyone's following the coding rule ... at least assuming that the
> target system isn't too insanely slow, and I've seen some pretty
> crazily overloaded machines. But if the coding rule is not being
> followed, then "the only reliable method of getting out of the problem
> is to blow things up and start over" becomes a dubious conclusion.

If the coding rule isn't being followed, a crash restart is the least of ones
problems... But that doesn't mean we shouldn't add infrastructure to make it
easier to detect violations of the spinlock rules - we've had lots of buglets
around this over the years ourselves, so we hardly can expect extension
authors to get this right. Particularly because we don't even document the
rules well, afair.

I think we should add cassert-only infrastructure tracking whether we
currently hold spinlocks, are in a signal handler and perhaps a few other
states. That'd allow us to add assertions like:

- no memory allocations when holding spinlocks or in signal handlers
- no lwlocks while holding spinlocks
- no lwlocks or spinlocks while in signal handlers

> Also, I wonder if many or even all uses of spinlocks uses ought to be
> replaced with either LWLocks or atomics. An LWLock might be slightly
> slower when contention is low, but it scales better when contention is
> high, displays a wait event so that you can see that you have
> contention if you do, and doesn't PANIC the system if the contention
> gets too bad. And an atomic will just be faster, in the cases where
> it's adequate.

I tried to replace all - unfortunately the results were not great. The problem
isn't primarily the lack of spinning (although it might be worth adding that
to lwlocks) or the cost of error recovery, the problem is that a reader-writer
lock are inherently more expensive than simpler locks that don't have multiple
levels.

One example of such increased overhead is that on x86 an lwlock unlock has to
be an atomic operation (to maintain the lock count), whereas as spinlock
unlock can just be a write + compiler barrier. Unfortunately the added atomic
operation turns out to matter in some performance critical cases like the
insertpos_lck.

I think we ought to split lwlocks into reader/writer and simpler mutex. The
simpler mutex still will be slower than s_lock in some relevant cases,
e.g. due to the error recovery handling, but it'd be "local" overhead, rather
than something affecting scalability.

FWIW, these days spinlocks do report a wait event when in perform_spin_delay()
- albeit without detail which lock is being held.

Greetings,

Andres Freund