Re: [PATCH] LWLock self-deadlock detection

On Wed, Nov 25, 2020 at 11:47 AM Craig Ringer
<craig.ringer@enterprisedb.com> wrote:
>> 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
rathernot 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
normalLOG (with backtrace if supported) then Assert(). So on an assert build we'd get a crash and core, and on a
non-assertbuild 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
withoutintroducing a new and really hard to test code path that'll also be surprising to callers. We probably don't
wantto 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 agree that it will be helpful to print something in the logs
indicating the reason for this hang in case the hang happens in a
production build. In your patch you have used ereport(PANIC, ) which
may simply be replaced by an Assert() in an assert enabled build. We
already have Assert(!LWLockHeldByMe()) so that should be safe. It will
be good to have -m immediate hint in LOG message. But it might just be
better to kill -9 that process to get rid of it. That will cause the
server to restart and not just shutdown.

--
Best Wishes,
Ashutosh Bapat