Fix hard to hit race condition in heapam's tuple locking code.

As mentioned in its commit message, eca0f1db left open a race condition,
where a page could be marked all-visible, after the code checked
PageIsAllVisible() to pin the VM, but before the page is locked.  Plug
that hole.

Reviewed-By: Robert Haas, Andres Freund
Author: Amit Kapila
Discussion: CAEepm=3fWAbWryVW9swHyLTY4sXVf0xbLvXqOwUoDiNCx9mBjQ@mail.gmail.com
Backpatch: -

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 38bba1629979e113001c61497eb5bf124b21df68..4acc62a550ea5e22de4dbbfe0f04d10546653c13 100644 (file)
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -4585,9 +4585,10 @@ heap_lock_tuple(Relation relation, HeapTuple tuple,
    block = ItemPointerGetBlockNumber(tid);
 
    /*
-    * Before locking the buffer, pin the visibility map page if it may be
-    * necessary. XXX: It might be possible for this to change after acquiring
-    * the lock below. We don't yet deal with that case.
+    * Before locking the buffer, pin the visibility map page if it appears to
+    * be necessary.  Since we haven't got the lock yet, someone else might be
+    * in the middle of changing this, so we'll need to recheck after we have
+    * the lock.
     */
    if (PageIsAllVisible(BufferGetPage(*buffer)))
        visibilitymap_pin(relation, block, &vmbuffer);
@@ -5075,6 +5076,23 @@ failed:
        goto out_locked;
    }
 
+   /*
+    * If we didn't pin the visibility map page and the page has become all
+    * visible while we were busy locking the buffer, or during some
+    * subsequent window during which we had it unlocked, we'll have to unlock
+    * and re-lock, to avoid holding the buffer lock across I/O.  That's a bit
+    * unfortunate, especially since we'll now have to recheck whether the
+    * tuple has been locked or updated under us, but hopefully it won't
+    * happen very often.
+    */
+   if (vmbuffer == InvalidBuffer && PageIsAllVisible(page))
+   {
+       LockBuffer(*buffer, BUFFER_LOCK_UNLOCK);
+       visibilitymap_pin(relation, block, &vmbuffer);
+       LockBuffer(*buffer, BUFFER_LOCK_EXCLUSIVE);
+       goto l3;
+   }
+
    xmax = HeapTupleHeaderGetRawXmax(tuple->t_data);
    old_infomask = tuple->t_data->t_infomask;
 
@@ -5665,9 +5683,10 @@ l4:
        CHECK_FOR_INTERRUPTS();
 
        /*
-        * Before locking the buffer, pin the visibility map page if it may be
-        * necessary.  XXX: It might be possible for this to change after
-        * acquiring the lock below. We don't yet deal with that case.
+        * Before locking the buffer, pin the visibility map page if it
+        * appears to be necessary.  Since we haven't got the lock yet,
+        * someone else might be in the middle of changing this, so we'll need
+        * to recheck after we have the lock.
         */
        if (PageIsAllVisible(BufferGetPage(buf)))
            visibilitymap_pin(rel, block, &vmbuffer);
@@ -5676,6 +5695,19 @@ l4:
 
        LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
 
+       /*
+        * If we didn't pin the visibility map page and the page has become
+        * all visible while we were busy locking the buffer, we'll have to
+        * unlock and re-lock, to avoid holding the buffer lock across I/O.
+        * That's a bit unfortunate, but hopefully shouldn't happen often.
+        */
+       if (vmbuffer == InvalidBuffer && PageIsAllVisible(BufferGetPage(buf)))
+       {
+           LockBuffer(buf, BUFFER_LOCK_UNLOCK);
+           visibilitymap_pin(rel, block, &vmbuffer);
+           LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
+       }
+
        /*
         * Check the tuple XMIN against prior XMAX, if any.  If we reached the
         * end of the chain, we're done, so return success.