Re: [PATCH] LWLock self-deadlock detection

Hi,

On 2020-11-27 20:22:41 +0200, Heikki Linnakangas wrote:
> On 26/11/2020 04:50, Tom Lane wrote:
> > Craig Ringer <craig(dot)ringer(at)enterprisedb(dot)com> writes:
> > > On Wed, Nov 25, 2020 at 9:23 PM Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>
> > > wrote:
> > > > I'd prefer to make the lock self deadlock check run for production
> > > > builds, not just cassert builds.
> >
> > I'd like to register a strong objection to spending any cycles whatsoever
> > on this. If you're using LWLocks in a way that could deadlock, you're
> > doing it wrong. The entire point of that mechanism is to be Light Weight
> > and not have all the overhead that the heavyweight lock mechanism has.
> > Building in deadlock checks and such is exactly the wrong direction.
> > If you think you need that, you should be using a heavyweight lock.
> >
> > Maybe there's some case for a cassert-only check of this sort, but running
> > it in production is right out.
> >
> > I'd also opine that checking for self-deadlock, but not any more general
> > deadlock, seems pretty pointless. Careless coding is far more likely
> > to create opportunities for the latter. (Thus, I have little use for
> > the existing assertions of this sort, either.)
>
> I've made the mistake of forgetting to release an LWLock many times, leading
> to self-deadlock. And I just reviewed a patch that did that this week [1].
> You usually find that mistake very quickly when you start testing though, I
> don't think I've seen it happen in production.

I think something roughly along Craig's original patch, basically adding
assert checks against holding a lock already, makes sense. Compared to
the other costs of running an assert build this isn't a huge cost, and
it's helpful.

I entirely concur that doing this outside of assertion builds is a
seriously bad idea.

Greetings,

Andres Freund