Thread: LockAcquireExtended() dontWait vs weaker lock levels than already held
Hi,
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.
However, in dontWait mode, there's no such path. Which means that
LockAcquireExtended() returns LOCKACQUIRE_NOT_AVAIL despite the fact that dontWait=false
would succeed in granting us the lock.
This seems decidedly suboptimal.
For one, the code flow to acquire a lock we already hold in some form is
unnecessarily hard to understand and expensive. There's no comment in
LockAcquireExtended() explaining that WaitOnLock() might immediately grant us
the lock in that case, we emit bogus TRACE_POSTGRESQL_LOCK_WAIT_START() etc.
For another, LockAcquireExtended(dontWait=true) returning spuriously is quite
confusing behaviour, and quite plausibly could cause bugs in fairly random
places.
Not planning to do anything about this for now, but I did want something I can
find if I hit such a problem in the future...
Greetings,
Andres Freund
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. -- Robert Haas EDB: http://www.enterprisedb.com
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
On Tue, Mar 22, 2022 at 3:01 PM Andres Freund <andres@anarazel.de> wrote: > 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 I don't mind you fixing the behavior. I just couldn't pass up an opportunity to complain about the structure of lock.c. -- Robert Haas EDB: http://www.enterprisedb.com