Re: [PATCH] LockAcquireExtended improvement

Hello Robert,
On 2024/3/8 1:02, Robert Haas wrote:
>
> 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?

Your version changes less code than mine by pushing the nowait flag down
into ProcSleep(). This looks fine in general, except for a little advice,
which I don't think there is necessary to add 'waiting' suffix to the
process name in function WaitOnLock with dontwait being true, as follows:

--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -1801,8 +1801,12 @@ WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner, bool dontWait)
LOCK_PRINT("WaitOnLock: sleeping on lock",
locallock->lock, locallock->tag.mode);

- /* adjust the process title to indicate that it's waiting */
- set_ps_display_suffix("waiting");
+ if (!dontWait)
+ {
+ /* adjust the process title to indicate that it's waiting */
+ set_ps_display_suffix("waiting");
+ }
+

awaitedLock = locallock;
awaitedOwner = owner;
@@ -1855,9 +1859,12 @@ WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner, bool dontWait)
{
/* In this path, awaitedLock remains set until LockErrorCleanup */

- /* reset ps display to remove the suffix */
- set_ps_display_remove_suffix();
-
+ if (!dontWait)
+ {
+ /* reset ps display to remove the suffix */
+ set_ps_display_remove_suffix();
+ }
+
/* and propagate the error */
PG_RE_THROW();
}
@@ -1865,8 +1872,11 @@ WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner, bool dontWait)

awaitedLock = NULL;

- /* reset ps display to remove the suffix */
- set_ps_display_remove_suffix();
+ if (!dontWait)
+ {
+ /* reset ps display to remove the suffix */
+ set_ps_display_remove_suffix();
+ }

LOCK_PRINT("WaitOnLock: wakeup on lock",
locallock->lock, locallock->tag.mode);

--
Jingxian Li