On Wed, Nov 25, 2020 at 11:47 AM Craig Ringer <craig.rin...@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 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 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
