Re: LockAcquireExtended() dontWait vs weaker lock levels than already held - Mailing list pgsql-hackers

Hi,

On 2022-03-22 14:20:55 -0400, Robert Haas wrote:
> On Tue, Mar 22, 2022 at 1:43 PM Andres Freund <andres@anarazel.de> wrote:
> > When LockAcquireExtended(dontWait=false) acquires a lock where we already hold
> > stronger lock and somebody else is also waiting for that lock, it goes through
> > a fairly circuitous path to acquire the lock:
> >
> > A conflicting lock is detected: if (lockMethodTable->conflictTab[lockmode] & lock->waitMask)
> > LockAcquireExtended() -> WaitOnLock() -> ProcSleep()
> > ProcSleep() follows this special path:
> >          * Special case: if I find I should go in front of some waiter, check to
> >          * see if I conflict with already-held locks or the requests before that
> >          * waiter.  If not, then just grant myself the requested lock immediately.
> > and grants the lock.
>
> I think this happens because lock.c is trying to imagine a world in
> which we don't know anything a priori about which locks are stronger
> or weaker than others and everything is deduced from the conflict
> matrix. I think at some point in time someone believed that we might
> use different conflict matrixes for different lock types. With an
> arbitrary conflict matrix, "stronger" and "weaker" aren't even
> necessarily well-defined ideas: A could conflict with B, B with C, and
> C with A, or something crazy like that. It seems rather unlikely to me
> that we'd ever do such a thing at this point. In fact, there are a lot
> of things in lock.c that we'd probably do differently if we were doing
> that work over.

We clearly already know how to compute whether a lock is "included" in
something we already hold - after all ProcSleep() successfully does so.

Isn't it a pretty trivial test? Seems like it'd boil down to something like

acquireMask = lockMethodTable->conflictTab[lockmode];
if ((MyProc->heldLocks & acquireMask) == acquireMask)
    /* already hold lock conflicting with it, grant the new lock to myself) */
else
    /* current behaviour */

LockCheckConflicts() mostly knows how to deal with this. It's just that we don't
even use LockCheckConflicts() if a lock acquisition conflicts with waitMask:

    /*
     * If lock requested conflicts with locks requested by waiters, must join
     * wait queue.  Otherwise, check for conflict with already-held locks.
     * (That's last because most complex check.)
     */
    if (lockMethodTable->conflictTab[lockmode] & lock->waitMask)
        found_conflict = true;
    else
        found_conflict = LockCheckConflicts(lockMethodTable, lockmode,
                                            lock, proclock);

Yes, there's more deadlocks that can be solved by queue reordering, but the
simple cases that ProcSleep() handles don't seem problematic to solve in
lock.c directly either...

Greetings,

Andres Freund