Re: [PATCH] LWLock self-deadlock detection

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.

So yeah, I agree it's not worth spending cycles on this. Maybe it would
be worth it if it's really simple to check, and you only do it after
waiting X seconds. (I haven't looked at this patch at all)

[1]
https://www.postgresql.org/message-id/8e1cda9b-1cb9-a634-d383-f757bf67b820@iki.fi

- Heikki