From: Tom Lane <tgl@sss.pgh.pa.us>
To: Peter Geoghegan <pg@bowt.ie>
cc: Andres Freund <andres@anarazel.de>,
        PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: PANIC: wrong buffer passed to visibilitymap_clear
In-reply-to: 
 <CAH2-WzkmFhemeqNvKKuf9orpSq8DOBjeMPN1G-=c+441hvr0NQ@mail.gmail.com>
References: <2247102.1618008027@sss.pgh.pa.us>
 <20210409232739.sazp5p4xnnagrusa@alap3.anarazel.de>
 <20210409233203.3khg3s5iw34g4xrc@alap3.anarazel.de>
 <2644575.1618117467@sss.pgh.pa.us>
 <CAH2-WzmWOLaWhQgEC1BJN0JZpOCOwxCAhjmd2Nd4yh4wrqju8Q@mail.gmail.com>
 <2679216.1618156670@sss.pgh.pa.us>
 <CAH2-WzmJ7THDU3jPgRH8J++b+pSoOh8nfGZEhNHh+4=LFatDeg@mail.gmail.com>
 <CAH2-WzkCXg6RQJdc_7ZGHcKvyoJAfFu9R2ZVAiE9mCcOfaXJdQ@mail.gmail.com>
 <2689873.1618161194@sss.pgh.pa.us> <2691106.1618162892@sss.pgh.pa.us>
 <2691724.1618163730@sss.pgh.pa.us>
 <CAH2-Wz=_u6ObPqg1Ar638U2CB6thvuni=StjLQ7iniuKX9YZdA@mail.gmail.com>
 <2692613.1618164979@sss.pgh.pa.us>
 <CAH2-WzkmFhemeqNvKKuf9orpSq8DOBjeMPN1G-=c+441hvr0NQ@mail.gmail.com>
Comments: In-reply-to Peter Geoghegan <pg@bowt.ie>
	message dated "Sun, 11 Apr 2021 11:28:22 -0700"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <2902079.1618244273.0@sss.pgh.pa.us>
Date: Mon, 12 Apr 2021 12:19:48 -0400
Message-ID: <2902168.1618244388@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2902168.1618244388%40sss.pgh.pa.us>
Precedence: bulk

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

Peter Geoghegan <pg@bowt.ie> writes:
> On Sun, Apr 11, 2021 at 11:16 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It wasn't very clear, because I hadn't thought it through very much;
>> but what I'm imagining is that we discard most of the thrashing around
>> all-visible rechecks and have just one such test somewhere very late
>> in heap_update, after we've successfully acquired a target buffer for
>> the update and are no longer going to possibly need to release any
>> buffer lock.  If at that one point we see the page is all-visible
>> and we don't have the vmbuffer, then we have to release all our locks
>> and go back to "l2".  Which is less efficient than some of the existing
>> code paths, but given how hard this problem is to reproduce, it seems
>> clear that optimizing for the occurrence is just not worth it.

> Oh! That sounds way better.

After poking at this for awhile, it seems like it won't work very nicely.
The problem is that once we've invoked the toaster, we really don't want
to just abandon that work; we'd leak any toasted out-of-line data that
was created.

So I think we have to stick with the current basic design, and just
tighten things up to make sure that visibility pins are accounted for
in the places that are missing it.

Hence, I propose the attached.  It passes check-world, but that proves
absolutely nothing of course :-(.  I wonder if there is any way to
exercise these code paths deterministically.

(I have realized BTW that I was exceedingly fortunate to reproduce
the buildfarm report here --- I have run hundreds of additional
cycles of the same test scenario without getting a second failure.)

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-missed-visibility-map-pins.patch";
	charset="us-ascii"
Content-ID: <2902079.1618244273.2@sss.pgh.pa.us>
Content-Description: fix-missed-visibility-map-pins.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/he=
apam.c
index 03d4abc938..265b31e981 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -3784,7 +3784,7 @@ l2:
 		 * overhead would be unchanged, that doesn't seem necessarily
 		 * worthwhile.
 		 */
-		if (PageIsAllVisible(BufferGetPage(buffer)) &&
+		if (PageIsAllVisible(page) &&
 			visibilitymap_clear(relation, block, vmbuffer,
 								VISIBILITYMAP_ALL_FROZEN))
 			cleared_all_frozen =3D true;
@@ -3846,36 +3846,46 @@ l2:
 		 * first".  To implement this, we must do RelationGetBufferForTuple
 		 * while not holding the lock on the old page, and we must rely on it
 		 * to get the locks on both pages in the correct order.
+		 *
+		 * Another consideration is that we need visibility map page pin(s) if
+		 * we will have to clear the all-visible flag on either page.  If we
+		 * call RelationGetBufferForTuple, we rely on it to acquire any such
+		 * pins; but if we don't, we have to handle that here.  Hence we need
+		 * a loop.
 		 */
-		if (newtupsize > pagefree)
-		{
-			/* Assume there's no chance to put heaptup on same page. */
-			newbuf =3D RelationGetBufferForTuple(relation, heaptup->t_len,
-											   buffer, 0, NULL,
-											   &vmbuffer_new, &vmbuffer);
-		}
-		else
+		for (;;)
 		{
+			if (newtupsize > pagefree)
+			{
+				/* It doesn't fit, must use RelationGetBufferForTuple. */
+				newbuf =3D RelationGetBufferForTuple(relation, heaptup->t_len,
+												   buffer, 0, NULL,
+												   &vmbuffer_new, &vmbuffer);
+				/* We're all done. */
+				break;
+			}
+			/* Acquire VM page pin if needed and we don't have it. */
+			if (vmbuffer =3D=3D InvalidBuffer && PageIsAllVisible(page))
+				visibilitymap_pin(relation, block, &vmbuffer);
 			/* Re-acquire the lock on the old tuple's page. */
 			LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
 			/* Re-check using the up-to-date free space */
 			pagefree =3D PageGetHeapFreeSpace(page);
-			if (newtupsize > pagefree)
+			if (newtupsize > pagefree ||
+				(vmbuffer =3D=3D InvalidBuffer && PageIsAllVisible(page)))
 			{
 				/*
-				 * Rats, it doesn't fit anymore.  We must now unlock and
-				 * relock to avoid deadlock.  Fortunately, this path should
-				 * seldom be taken.
+				 * Rats, it doesn't fit anymore, or somebody just now set the
+				 * all-visible flag.  We must now unlock and loop to avoid
+				 * deadlock.  Fortunately, this path should seldom be taken.
 				 */
 				LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
-				newbuf =3D RelationGetBufferForTuple(relation, heaptup->t_len,
-												   buffer, 0, NULL,
-												   &vmbuffer_new, &vmbuffer);
 			}
 			else
 			{
-				/* OK, it fits here, so we're done. */
+				/* We're all done. */
 				newbuf =3D buffer;
+				break;
 			}
 		}
 	}
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index 37a1be4114..ffc89685bf 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -293,9 +293,13 @@ RelationAddExtraBlocks(Relation relation, BulkInsertS=
tate bistate)
  *	happen if space is freed in that page after heap_update finds there's =
not
  *	enough there).  In that case, the page will be pinned and locked only =
once.
  *
- *	For the vmbuffer and vmbuffer_other arguments, we avoid deadlock by
- *	locking them only after locking the corresponding heap page, and takin=
g
- *	no further lwlocks while they are locked.
+ *	We also handle the possibility that the all-visible flag will need to =
be
+ *	cleared on one or both pages.  If so, pin on the associated visibility=
 map
+ *	page must be acquired before acquiring buffer lock(s), to avoid possib=
ly
+ *	doing I/O while holding buffer locks.  The pins are passed back to the
+ *	caller using the input-output arguments vmbuffer and vmbuffer_other.
+ *	Note that in some cases the caller might have already acquired such pi=
ns,
+ *	which is indicated by these arguments not being InvalidBuffer on entry=
.
  *
  *	We normally use FSM to help us find free space.  However,
  *	if HEAP_INSERT_SKIP_FSM is specified, we just append a new empty page =
to
@@ -666,6 +670,8 @@ loop:
 	if (otherBuffer !=3D InvalidBuffer)
 	{
 		Assert(otherBuffer !=3D buffer);
+		targetBlock =3D BufferGetBlockNumber(buffer);
+		Assert(targetBlock > otherBlock);
 =

 		if (unlikely(!ConditionalLockBuffer(otherBuffer)))
 		{
@@ -674,10 +680,16 @@ loop:
 			LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
 =

 			/*
-			 * Because the buffer was unlocked for a while, it's possible,
-			 * although unlikely, that the page was filled. If so, just retry
-			 * from start.
+			 * 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);

------- =_aaaaaaaaaa0--