Re: [PATCH] LockAcquireExtended improvement

Hello Jingxian Li!

I agree with you that this behavior seems surprising. I don't think
it's quite a bug, more of a limitation. However, I think it would be
nice to fix it if we can find a good way to do that.

On Wed, Nov 29, 2023 at 10:43 PM Jingxian Li <aqktjcm(at)qq(dot)com> wrote:
> Transaction A already holds an n-mode lock on table test,
> that is, there is no locks held conflicting with the n-mode lock on table test,
> If then transaction A requests an m-mode lock on table test,
> as n's confilctTab covers m, it can be concluded that
> there are no locks conflicting with the requested m-mode lock.

This algorithm seems correct to me, but I think Andres is right to be
concerned about overhead. You're proposing to inject a call to
CheckLocalLockConflictTabCover() into the main code path of
LockAcquireExtended(), so practically every lock acquisition will pay
the cost of that function. And that function is not particularly
cheap: every call to LockHeldByMe is a hash table lookup. That sounds
pretty painful. If we could incur the overhead of this only when we
knew for certain that we would otherwise have to fail, that would be
more palatable, but doing it on every non-fastpath heavyweight lock
acquisition seems way too expensive.

Even aside from overhead, the approach the patch takes doesn't seem
quite right to me. As you noted, ProcSleep() has logic to jump the
queue if adding ourselves at the end would inevitably result in
deadlock, which is why your test case doesn't need to wait until
deadlock_timeout for the lock acquisition to succeed. But because that
logic happens in ProcSleep(), it's not reached in the NOWAIT case,
which means that it doesn't help any more once NOWAIT is specified. I
think that the right way to fix the problem would be to reach that
check even in the NOWAIT case, which could be done either by hoisting
some of the logic in ProcSleep() up into LockAcquireExtended(), or by
pushing the nowait flag down into ProcSleep() so that we can fail only
if we're definitely going to sleep. The former seems more elegant in
theory, but the latter looks easier to implement, at least at first
glance.

But the patch as proposed instead invents a new way of making the test
case work, not leveraging the existing logic and, I suspect, not
matching the behavior in all cases.

I also agree with Vignesh that a test case would be a good idea. It
would need to be an isolation test, since the regular regression
tester isn't powerful enough for this (at least, I don't see how to
make it work).

I hope that this input is helpful to you.

--
Robert Haas
EDB: http://www.enterprisedb.com