Re: [PATCH] LockAcquireExtended improvement

On Thu, Feb 8, 2024 at 5:28 AM Jingxian Li <aqktjcm(at)qq(dot)com> wrote:
> Based on your comments above, I improve the commit message and comment for
> InsertSelfIntoWaitQueue in new patch.

Well, I had a look at this patch today, and even after reading the new
commit message, I couldn't really convince myself that it was correct.
It may well be entirely correct, but I simply find it hard to tell. It
would help if the comments had been adjusted a bit more, e.g.

/* Skip the wait and just
grant myself the lock. */
- GrantLock(lock, proclock, lockmode);
- GrantAwaitedLock();
return PROC_WAIT_STATUS_OK;

Surely this is not an acceptable change. The comments says "and just
grant myself the lock" but the code no longer does that.

But instead of just complaining, I decided to try writing a version of
the patch that seemed acceptable to me. Here it is. I took a different
approach than you. Instead of splitting up ProcSleep(), I just passed
down the dontWait flag to WaitOnLock() and ProcSleep(). In
LockAcquireExtended(), I moved the existing code that handles giving
up in the don't-wait case from before the call to ProcSleep() to
afterward. As far as I can see, the major way this could be wrong is
if calling ProcSleep() with dontWait = true and having it fail to
acquire the lock changes the state in some way that makes the cleanup
code that I moved incorrect. I don't *think* that's the case, but I
might be wrong.

What do you think of this version?

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