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