From: Tom Lane <tgl@sss.pgh.pa.us>
To: Jaime Casanova <jcasanov@systemguards.com.ec>
cc: pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Avoid improbable PANIC during heap_update.
In-reply-to: <692791.1664573331@sss.pgh.pa.us>
References: <E1lWLjP-00006Y-Ml@gemulon.postgresql.org>
 <YzVPfCKNrmi0aLEh@ahch-to> <YzdVJnQocQ6RHKxk@ahch-to>
 <692791.1664573331@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Fri, 30 Sep 2022 17:28:51 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <741194.1664574731.0@sss.pgh.pa.us>
Date: Fri, 30 Sep 2022 17:52:30 -0400
Message-ID: <741234.1664574750@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/741234.1664574750%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <741194.1664574731.1@sss.pgh.pa.us>

I wrote:
> Ugh ... I think I see the problem.  There's still one path through
> RelationGetBufferForTuple that fails to guarantee that it's acquired
> a vmbuffer pin if the all-visible flag becomes set in the otherBuffer.
> Namely, if we're forced to extend the relation, then we deal with
> vm pins when ConditionalLockBuffer(otherBuffer) fails ... but not
> when it succeeds.  I think the fix is just to move the last
> GetVisibilityMapPins call out of the "if
> (unlikely(!ConditionalLockBuffer(otherBuffer)))" stanza.

Concretely, about like this.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="fix-RelationGetBufferForTuple-some-more.patch";
	charset="us-ascii"
Content-ID: <741194.1664574731.2@sss.pgh.pa.us>
Content-Description: fix-RelationGetBufferForTuple-some-more.patch

diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index ae2e2ce37a..b0ece66629 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -678,29 +678,34 @@ loop:
 			LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
 			LockBuffer(otherBuffer, BUFFER_LOCK_EXCLUSIVE);
 			LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
+		}
 
-			/*
-			 * Because the buffers were unlocked for a while, it's possible,
-			 * although unlikely, that an all-visible flag became set or that
-			 * somebody used up the available space in the new page.  We can
-			 * use GetVisibilityMapPins to deal with the first case.  In the
-			 * second case, just retry from start.
-			 */
-			GetVisibilityMapPins(relation, otherBuffer, buffer,
-								 otherBlock, targetBlock, vmbuffer_other,
-								 vmbuffer);
+		/*
+		 * Because the buffers were unlocked for a while, it's possible,
+		 * although unlikely, that an all-visible flag became set or that
+		 * somebody used up the available space in the new page.  We can use
+		 * GetVisibilityMapPins to deal with the first case.  In the second
+		 * case, just retry from start.
+		 */
+		GetVisibilityMapPins(relation, otherBuffer, buffer,
+							 otherBlock, targetBlock, vmbuffer_other,
+							 vmbuffer);
 
-			if (len > PageGetHeapFreeSpace(page))
-			{
-				LockBuffer(otherBuffer, BUFFER_LOCK_UNLOCK);
-				UnlockReleaseBuffer(buffer);
+		/*
+		 * Note that we have to check the available space even if our
+		 * conditional lock succeeded, because GetVisibilityMapPins might've
+		 * transiently released lock on the target buffer to acquire a VM pin
+		 * for the otherBuffer.
+		 */
+		if (len > PageGetHeapFreeSpace(page))
+		{
+			LockBuffer(otherBuffer, BUFFER_LOCK_UNLOCK);
+			UnlockReleaseBuffer(buffer);
 
-				goto loop;
-			}
+			goto loop;
 		}
 	}
-
-	if (len > PageGetHeapFreeSpace(page))
+	else if (len > PageGetHeapFreeSpace(page))
 	{
 		/* We should not get here given the test at the top */
 		elog(PANIC, "tuple is too big: size %zu", len);

------- =_aaaaaaaaaa0--