Use UnlockReleaseBuffer() in more places

An upcoming commit will make UnlockReleaseBuffer() considerably faster and
more scalable than doing LockBuffer(BUFFER_LOCK_UNLOCK); ReleaseBuffer();. But
it's a small performance benefit even as-is.

Most of the callsites changed in this patch are not performance sensitive,
however some, like the nbtree ones, are in critical paths.

This patch changes all the easily convertible places over to
UnlockReleaseBuffer() mainly because I needed to check all of them anyway, and
reducing cases where the operations are done separately makes the checking
easier.

Reviewed-by: Melanie Plageman <melanieplageman@gmail.com>
Discussion: https://postgr.es/m/5ubipyssiju5twkb7zgqwdr7q2vhpkpmuelxfpanetlk6ofnop@hvxb4g2amb2d

diff --git a/contrib/amcheck/verify_gin.c b/contrib/amcheck/verify_gin.c
index f7a15a21467eefb851374cfe10415603ab09e050..abfad07d5e48f9615ba4a0e5c2b3f37655845af2 100644 (file)
--- a/contrib/amcheck/verify_gin.c
+++ b/contrib/amcheck/verify_gin.c
@@ -368,8 +368,7 @@ gin_check_posting_tree_parent_keys_consistency(Relation rel, BlockNumber posting
                                stack->next = ptr;
                        }
                }
-               LockBuffer(buffer, GIN_UNLOCK);
-               ReleaseBuffer(buffer);
+               UnlockReleaseBuffer(buffer);
 
                /* Step to next item in the queue */
                stack_next = stack->next;
@@ -642,8 +641,7 @@ gin_check_parent_keys_consistency(Relation rel,
                        prev_attnum = current_attnum;
                }
 
-               LockBuffer(buffer, GIN_UNLOCK);
-               ReleaseBuffer(buffer);
+               UnlockReleaseBuffer(buffer);
 
                /* Step to next item in the queue */
                stack_next = stack->next;

diff --git a/contrib/pageinspect/rawpage.c b/contrib/pageinspect/rawpage.c
index 86fe245cac5e5ff9a00a2058fa8285b7a62ad95e..f3996dc39fcdedc6168379d4227150b6c39f5814 100644 (file)
--- a/contrib/pageinspect/rawpage.c
+++ b/contrib/pageinspect/rawpage.c
@@ -193,8 +193,7 @@ get_raw_page_internal(text *relname, ForkNumber forknum, BlockNumber blkno)
 
        memcpy(raw_page_data, BufferGetPage(buf), BLCKSZ);
 
-       LockBuffer(buf, BUFFER_LOCK_UNLOCK);
-       ReleaseBuffer(buf);
+       UnlockReleaseBuffer(buf);
 
        relation_close(rel, AccessShareLock);
 

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 044f385e477e647c5259b79755deacf373a0275b..eb1f67f31cd70c998c1030523483f2174e072cbc 100644 (file)
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1697,8 +1697,7 @@ heap_fetch(Relation relation,
        offnum = ItemPointerGetOffsetNumber(tid);
        if (offnum < FirstOffsetNumber || offnum > PageGetMaxOffsetNumber(page))
        {
-               LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
-               ReleaseBuffer(buffer);
+               UnlockReleaseBuffer(buffer);
                *userbuf = InvalidBuffer;
                tuple->t_data = NULL;
                return false;
@@ -1714,8 +1713,7 @@ heap_fetch(Relation relation,
         */
        if (!ItemIdIsNormal(lp))
        {
-               LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
-               ReleaseBuffer(buffer);
+               UnlockReleaseBuffer(buffer);
                *userbuf = InvalidBuffer;
                tuple->t_data = NULL;
                return false;

diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index d26ceacd38ceb5b40e99b648eab09ee03c00c639..1097f44a74efe786434d9b84a951370c0eabc4ea 100644 (file)
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -711,14 +711,15 @@ loop:
                 * unlock the two buffers in, so this can be slightly simpler than the
                 * code above.
                 */
-               LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
                if (otherBuffer == InvalidBuffer)
-                       ReleaseBuffer(buffer);
+                       UnlockReleaseBuffer(buffer);
                else if (otherBlock != targetBlock)
                {
+                       UnlockReleaseBuffer(buffer);
                        LockBuffer(otherBuffer, BUFFER_LOCK_UNLOCK);
-                       ReleaseBuffer(buffer);
                }
+               else
+                       LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
 
                /* Is there an ongoing bulk extension? */
                if (bistate && bistate->next_free != InvalidBlockNumber)

diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index cc9c45dc40c083b7c6d252fc8d8b87ce2e684282..0547038616e045bf19358acb9c1b282dae453bf2 100644 (file)
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -1011,24 +1011,47 @@ _bt_relandgetbuf(Relation rel, Buffer obuf, BlockNumber blkno, int access)
 
        Assert(BlockNumberIsValid(blkno));
        if (BufferIsValid(obuf))
-               _bt_unlockbuf(rel, obuf);
-       buf = ReleaseAndReadBuffer(obuf, rel, blkno);
-       _bt_lockbuf(rel, buf, access);
+       {
+               if (BufferGetBlockNumber(obuf) == blkno)
+               {
+                       /* trade in old lock mode for new lock */
+                       _bt_unlockbuf(rel, obuf);
+                       buf = obuf;
+               }
+               else
+               {
+                       /* release lock and pin at once, that's a bit more efficient */
+                       _bt_relbuf(rel, obuf);
+                       buf = ReadBuffer(rel, blkno);
+               }
+       }
+       else
+               buf = ReadBuffer(rel, blkno);
 
+       _bt_lockbuf(rel, buf, access);
        _bt_checkpage(rel, buf);
+
        return buf;
 }
 
 /*
  *     _bt_relbuf() -- release a locked buffer.
  *
- * Lock and pin (refcount) are both dropped.
+ * Lock and pin (refcount) are both dropped. This is a bit more efficient than
+ * doing the two operations separately.
  */
 void
 _bt_relbuf(Relation rel, Buffer buf)
 {
-       _bt_unlockbuf(rel, buf);
-       ReleaseBuffer(buf);
+       /*
+        * Buffer is pinned and locked, which means that it is expected to be
+        * defined and addressable.  Check that proactively.
+        */
+       VALGRIND_CHECK_MEM_IS_DEFINED(BufferGetPage(buf), BLCKSZ);
+       if (!RelationUsesLocalBuffers(rel))
+               VALGRIND_MAKE_MEM_NOACCESS(BufferGetPage(buf), BLCKSZ);
+
+       UnlockReleaseBuffer(buf);
 }
 
 /*

diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index f4f07c9a1c08446b26e51ee75d8a9283dfca1f9e..8a8c4adb9e1eead250347ede16165ef7c6ef25f4 100644 (file)
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -2561,8 +2561,7 @@ again:
                        XLogNeedsFlush(BufferGetLSN(buf_hdr)) &&
                        StrategyRejectBuffer(strategy, buf_hdr, from_ring))
                {
-                       LockBuffer(buf, BUFFER_LOCK_UNLOCK);
-                       UnpinBuffer(buf_hdr);
+                       UnlockReleaseBuffer(buf);
                        goto again;
                }
 

diff --git a/src/backend/storage/freespace/freespace.c b/src/backend/storage/freespace/freespace.c
index b9a8f368a63720b13e780190f595c18cb51af7f4..40d67a96178bc2d3b18c3f007984a8ca364fb118 100644 (file)
--- a/src/backend/storage/freespace/freespace.c
+++ b/src/backend/storage/freespace/freespace.c
@@ -915,9 +915,8 @@ fsm_vacuum_page(Relation rel, FSMAddress addr,
                ((FSMPage) PageGetContents(page))->fp_next_slot = 0;
                BufferFinishSetHintBits(buf, false, false);
        }
-       LockBuffer(buf, BUFFER_LOCK_UNLOCK);
 
-       ReleaseBuffer(buf);
+       UnlockReleaseBuffer(buf);
 
        return max_avail;
 }

diff --git a/src/test/modules/test_aio/test_aio.c b/src/test/modules/test_aio/test_aio.c
index 2ae4a559fab0a797994fc26cfa2efe83f7f3209e..138e1259dfdbf8368b908dfea632cd9a91e4337c 100644 (file)
--- a/src/test/modules/test_aio/test_aio.c
+++ b/src/test/modules/test_aio/test_aio.c
@@ -221,9 +221,7 @@ modify_rel_block(PG_FUNCTION_ARGS)
         */
        memcpy(page, BufferGetPage(buf), BLCKSZ);
 
-       LockBuffer(buf, BUFFER_LOCK_UNLOCK);
-
-       ReleaseBuffer(buf);
+       UnlockReleaseBuffer(buf);
 
        /*
         * Don't want to have a buffer in-memory that's marked valid where the
@@ -496,8 +494,7 @@ invalidate_rel_block(PG_FUNCTION_ARGS)
                                else
                                        FlushOneBuffer(buf);
                        }
-                       LockBuffer(buf, BUFFER_LOCK_UNLOCK);
-                       ReleaseBuffer(buf);
+                       UnlockReleaseBuffer(buf);
 
                        if (BufferIsLocal(buf))
                                InvalidateLocalBuffer(GetLocalBufferDescriptor(-buf - 1), true);