Subject: Re: [PATCH] LockAcquireExtended improvement

From: "Jingxian Li" <[email-masked]>
Date: Tue, 12 Mar 2024 11:11:33 +0800

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

---

From: Robert Haas <[email-masked]>
Date: Tue, 12 Mar 2024 09:33:23 -0400

> process name in function WaitOnLock with dontwait being true, as follows:

That could be done, but in my opinion it's not necessary. The waiting
suffix will appear only very briefly, and probably only in relatively
rare cases. It doesn't seem worth adding code to avoid it.

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

---

From: Robert Haas <[email-masked]>
Date: Thu, 14 Mar 2024 09:15:56 -0400

> rare cases. It doesn't seem worth adding code to avoid it.

Seeing no further discussion, I have committed my version of this
patch, with your test case.

Thanks for pursuing this improvement!

---

From: Will Mortensen <[email-masked]>
Date: Tue, 26 Mar 2024 19:14:32 -0700

> patch, with your test case.

This comment on ProcSleep() seems to have the values of dontWait
backward (double negatives are tricky):

    * Result: PROC_WAIT_STATUS_OK if we acquired the lock,
PROC_WAIT_STATUS_ERROR
    * if not (if dontWait = true, this is a deadlock; if dontWait = false, we
    * would have had to wait).

Also there's a minor typo in a comment in LockAcquireExtended():

    * Check the proclock entry status. If dontWait = true, this is an
    * expected case; otherwise, it will open happen if something in the
    * ipc communication doesn't work correctly.

"open" should be "only".

---

From: Will Mortensen <[email-masked]>
Date: Fri, 17 May 2024 23:38:35 -0700

>     * ipc communication doesn't work correctly.

Here's a patch fixing those typos.

---

From: "Jingxian Li" <[email-masked]>
Date: Sat, 18 May 2024 17:10:38 +0800

> Here's a patch fixing those typos.

Nice catch! The patch looks good to me.

--
Jingxian Li

---

From: Michael Paquier <[email-masked]>
Date: Sat, 18 May 2024 20:37:35 +0900

> Here's a patch fixing those typos.

Perhaps, this, err..  Should not have been named "dontWait" but
"doWait" ;)

Anyway, this goes way back in time and it is deep in the stack
(LockAcquireExtended, etc.) so it is too late to change: the patch
should be OK as it is.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Thu, 23 May 2024 13:58:28 +0900

> Nice catch! The patch looks good to me.

And fixed that as well.
--
Michael

---

From: Will Mortensen <[email-masked]>
Date: Wed, 22 May 2024 22:21:53 -0700

Thanks! :-)