This looks useful. LWLockCheckSelfDeadlock() could use LWLockHeldByMe variant instead of copying that code with possibly a change in that function to return the required information.
Yes, possibly so. I was reluctant to mess with the rest of the code too much.
I am also seeing a pattern Assert(!LWLockHeldByMe()); LWLockAcquire()
at some places. Should we change LWLockAcquire to do Assert(!LWLockHeldByMe()) always to detect such occurrences?
I'm inclined not to, at least not without benchmarking it, because that'd do the check before we attempt the fast-path. cassert builds are still supposed to perform decently and be suitable for day to day development and I'd rather not risk a slowdown.
I'd prefer to make the lock self deadlock check run for production builds, not just cassert builds. It'd print a normal LOG (with backtrace if supported) then Assert(). So on an assert build we'd get a crash and core, and on a non-assert build we'd carry on and self-deadlock anyway.
That's probably the safest thing to do. We can't expect to safely ERROR out of the middle of an LWLockAcquire(), not without introducing a new and really hard to test code path that'll also be surprising to callers. We probably don't want to PANIC the whole server for non-assert builds since it might enter a panic-loop. So it's probably better to self-deadlock. We could HINT that a -m immediate shutdown will be necessary to recover though.
I don't think it makes sense to introduce much complexity for a feature that's mainly there to help developers and to catch corner-case bugs.
(FWIW a variant of this patch has been in 2ndQPostgres for some time. It helped catch an extension bug just the other day.)