Re: pgsql: Avoid improbable PANIC during heap_update. - Mailing list pgsql-committers

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

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);