Fix VM buffer pin management in heap_lock_updated_tuple_rec().

Sloppy coding in this function could lead to leaking a VM buffer pin,
or to attempting to free the same pin twice.  Repair.  While at it,
reduce the code's tendency to free and reacquire the same page pin.

Back-patch to 9.6; before that, this routine did not concern itself
with VM pages.

Amit Kapila and Tom Lane

Discussion: https://postgr.es/m/CAA4eK1KJKwhc=isgTQHjM76CAdVswzNeAuZkh_cx-6QgGkSEgA@mail.gmail.com

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index dc762f913d2503579e875c31d58a82d898b87b0a..302f8c63ad638572e621bec0d470a373ad524d8d 100644 (file)
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -5677,6 +5677,7 @@ heap_lock_updated_tuple_rec(Relation rel, ItemPointer tid, TransactionId xid,
                new_xmax;
    TransactionId priorXmax = InvalidTransactionId;
    bool        cleared_all_frozen = false;
+   bool        pinned_desired_page;
    Buffer      vmbuffer = InvalidBuffer;
    BlockNumber block;
 
@@ -5698,7 +5699,8 @@ heap_lock_updated_tuple_rec(Relation rel, ItemPointer tid, TransactionId xid,
             * chain, and there's no further tuple to lock: return success to
             * caller.
             */
-           return HeapTupleMayBeUpdated;
+           result = HeapTupleMayBeUpdated;
+           goto out_unlocked;
        }
 
 l4:
@@ -5711,9 +5713,12 @@ l4:
         * to recheck after we have the lock.
         */
        if (PageIsAllVisible(BufferGetPage(buf)))
+       {
            visibilitymap_pin(rel, block, &vmbuffer);
+           pinned_desired_page = true;
+       }
        else
-           vmbuffer = InvalidBuffer;
+           pinned_desired_page = false;
 
        LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
 
@@ -5722,8 +5727,13 @@ l4:
         * 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.
+        *
+        * Note: in some paths through this function, we will reach here
+        * holding a pin on a vm page that may or may not be the one matching
+        * this page.  If this page isn't all-visible, we won't use the vm
+        * page, but we hold onto such a pin till the end of the function.
         */
-       if (vmbuffer == InvalidBuffer && PageIsAllVisible(BufferGetPage(buf)))
+       if (!pinned_desired_page && PageIsAllVisible(BufferGetPage(buf)))
        {
            LockBuffer(buf, BUFFER_LOCK_UNLOCK);
            visibilitymap_pin(rel, block, &vmbuffer);
@@ -5749,8 +5759,8 @@ l4:
         */
        if (TransactionIdDidAbort(HeapTupleHeaderGetXmin(mytup.t_data)))
        {
-           UnlockReleaseBuffer(buf);
-           return HeapTupleMayBeUpdated;
+           result = HeapTupleMayBeUpdated;
+           goto out_locked;
        }
 
        old_infomask = mytup.t_data->t_infomask;
@@ -5957,8 +5967,6 @@ next:
        priorXmax = HeapTupleHeaderGetUpdateXid(mytup.t_data);
        ItemPointerCopy(&(mytup.t_data->t_ctid), &tupid);
        UnlockReleaseBuffer(buf);
-       if (vmbuffer != InvalidBuffer)
-           ReleaseBuffer(vmbuffer);
    }
 
    result = HeapTupleMayBeUpdated;
@@ -5966,11 +5974,11 @@ next:
 out_locked:
    UnlockReleaseBuffer(buf);
 
+out_unlocked:
    if (vmbuffer != InvalidBuffer)
        ReleaseBuffer(vmbuffer);
 
    return result;
-
 }
 
 /*