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