Re: [PATCH] LWLock self-deadlock detection

On Wed, 30 Dec 2020 at 10:11, Andres Freund <andres(at)anarazel(dot)de> wrote:

> 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.
>

Yeah, given it only targets developer error that's sensible.