v20250807-0002-nbtree-Use-ReadRecentBuffer-in_bt_getroot.patch

From 730467cfcf1d1b7f0a22f61bd48a37371f80cce9 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 27 Jun 2023 14:07:34 -0700
Subject: [PATCH v20250807 02/11] nbtree: Use ReadRecentBuffer()
 in_bt_getroot()

We tend to access the btree root page over and over, when descending the
index. It's quite expensive. Not really specific to NUMA, but it hurts
there much more.

Thomas Munro worked on this.

Discussion: https://www.postgresql.org/message-id/20230627020546.t6z4tntmj7wmjrfh%40awork3.anarazel.de
Discussion: https://www.postgresql.org/message-id/CA%2BhUKGJ8N_DRSB0YioinWjS2ycMpmOLy32mbBqVVztwBvXgyJA%40mail.gmail.com
---
 src/backend/access/nbtree/nbtpage.c   | 21 ++++++++++++++--
 src/backend/access/nbtree/nbtsearch.c | 17 +++++++++----
 src/backend/storage/buffer/bufmgr.c   | 35 +++++----------------------
 src/include/utils/backend_status.h    |  4 +++
 src/include/utils/rel.h               |  4 +++
 5 files changed, 45 insertions(+), 36 deletions(-)

diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index c79dd38ee18..3cc26490f06 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -35,6 +35,7 @@
 #include "storage/procarray.h"
 #include "utils/memdebug.h"
 #include "utils/memutils.h"
+#include "utils/rel.h"
 #include "utils/snapmgr.h"
 
 static BTMetaPageData *_bt_getmeta(Relation rel, Buffer metabuf);
@@ -344,7 +345,7 @@ Buffer
 _bt_getroot(Relation rel, Relation heaprel, int access)
 {
 	Buffer		metabuf;
-	Buffer		rootbuf;
+	Buffer		rootbuf = InvalidBuffer;
 	Page		rootpage;
 	BTPageOpaque rootopaque;
 	BlockNumber rootblkno;
@@ -373,7 +374,20 @@ _bt_getroot(Relation rel, Relation heaprel, int access)
 		Assert(rootblkno != P_NONE);
 		rootlevel = metad->btm_fastlevel;
 
-		rootbuf = _bt_getbuf(rel, rootblkno, BT_READ);
+
+		if (BufferIsValid(rel->rd_recent_root))
+		{
+			if (ReadRecentBuffer(rel->rd_locator, MAIN_FORKNUM, rootblkno,
+								 rel->rd_recent_root))
+			{
+				rootbuf = rel->rd_recent_root;
+				_bt_lockbuf(rel, rootbuf, BT_READ);
+				_bt_checkpage(rel, rootbuf);
+			}
+		}
+
+		if (rootbuf == InvalidBuffer)
+			rootbuf = _bt_getbuf(rel, rootblkno, BT_READ);
 		rootpage = BufferGetPage(rootbuf);
 		rootopaque = BTPageGetOpaque(rootpage);
 
@@ -390,6 +404,7 @@ _bt_getroot(Relation rel, Relation heaprel, int access)
 			P_RIGHTMOST(rootopaque))
 		{
 			/* OK, accept cached page as the root */
+			rel->rd_recent_root = rootbuf;
 			return rootbuf;
 		}
 		_bt_relbuf(rel, rootbuf);
@@ -555,6 +570,8 @@ _bt_getroot(Relation rel, Relation heaprel, int access)
 				 rootopaque->btpo_level, rootlevel);
 	}
 
+	rel->rd_recent_root = rootbuf;
+
 	/*
 	 * By here, we have a pin and read lock on the root page, and no lock set
 	 * on the metadata page.  Return the root page's buffer.
diff --git a/src/backend/access/nbtree/nbtsearch.c b/src/backend/access/nbtree/nbtsearch.c
index d69798795b4..e1e73b4aaf3 100644
--- a/src/backend/access/nbtree/nbtsearch.c
+++ b/src/backend/access/nbtree/nbtsearch.c
@@ -168,11 +168,17 @@ _bt_search(Relation rel, Relation heaprel, BTScanInsert key, Buffer *bufP,
 		 * stack entry for this page/level.  If caller ends up splitting a
 		 * page one level down, it usually ends up inserting a new pivot
 		 * tuple/downlink immediately after the location recorded here.
+		 *
+		 * FIXME: Unfortunately this isn't a usable gating condition, as
+		 * vacuum uses BT_READ and needs the stack.
 		 */
-		new_stack = (BTStack) palloc(sizeof(BTStackData));
-		new_stack->bts_blkno = BufferGetBlockNumber(*bufP);
-		new_stack->bts_offset = offnum;
-		new_stack->bts_parent = stack_in;
+		if (false && access == BT_WRITE)
+		{
+			new_stack = (BTStack) palloc(sizeof(BTStackData));
+			new_stack->bts_blkno = BufferGetBlockNumber(*bufP);
+			new_stack->bts_offset = offnum;
+			new_stack->bts_parent = stack_in;
+		}
 
 		/*
 		 * Page level 1 is lowest non-leaf page level prior to leaves.  So, if
@@ -186,7 +192,8 @@ _bt_search(Relation rel, Relation heaprel, BTScanInsert key, Buffer *bufP,
 		*bufP = _bt_relandgetbuf(rel, *bufP, child, page_access);
 
 		/* okay, all set to move down a level */
-		stack_in = new_stack;
+		if (false && access == BT_WRITE)
+			stack_in = new_stack;
 	}
 
 	/*
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 67431208e7f..bd50535385f 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -685,7 +685,6 @@ ReadRecentBuffer(RelFileLocator rlocator, ForkNumber forkNum, BlockNumber blockN
 	BufferDesc *bufHdr;
 	BufferTag	tag;
 	uint32		buf_state;
-	bool		have_private_ref;
 
 	Assert(BufferIsValid(recent_buffer));
 
@@ -713,38 +712,16 @@ ReadRecentBuffer(RelFileLocator rlocator, ForkNumber forkNum, BlockNumber blockN
 	else
 	{
 		bufHdr = GetBufferDescriptor(recent_buffer - 1);
-		have_private_ref = GetPrivateRefCount(recent_buffer) > 0;
 
-		/*
-		 * Do we already have this buffer pinned with a private reference?  If
-		 * so, it must be valid and it is safe to check the tag without
-		 * locking.  If not, we have to lock the header first and then check.
-		 */
-		if (have_private_ref)
-			buf_state = pg_atomic_read_u32(&bufHdr->state);
-		else
-			buf_state = LockBufHdr(bufHdr);
-
-		if ((buf_state & BM_VALID) && BufferTagsEqual(&tag, &bufHdr->tag))
+		if (!PinBuffer(bufHdr, NULL) ||
+			!BufferTagsEqual(&tag, &bufHdr->tag))
 		{
-			/*
-			 * It's now safe to pin the buffer.  We can't pin first and ask
-			 * questions later, because it might confuse code paths like
-			 * InvalidateBuffer() if we pinned a random non-matching buffer.
-			 */
-			if (have_private_ref)
-				PinBuffer(bufHdr, NULL);	/* bump pin count */
-			else
-				PinBuffer_Locked(bufHdr);	/* pin for first time */
-
-			pgBufferUsage.shared_blks_hit++;
-
-			return true;
+			UnpinBuffer(bufHdr);
+			return false;
 		}
 
-		/* If we locked the header above, now unlock. */
-		if (!have_private_ref)
-			UnlockBufHdr(bufHdr, buf_state);
+		pgBufferUsage.shared_blks_hit++;
+		return true;
 	}
 
 	return false;
diff --git a/src/include/utils/backend_status.h b/src/include/utils/backend_status.h
index 3016501ac05..443c492cf34 100644
--- a/src/include/utils/backend_status.h
+++ b/src/include/utils/backend_status.h
@@ -97,6 +97,10 @@ typedef struct PgBackendGSSStatus
  */
 typedef struct PgBackendStatus
 {
+#ifdef pg_attribute_aligned
+	pg_attribute_aligned(PG_CACHE_LINE_SIZE)
+#endif
+
 	/*
 	 * To avoid locking overhead, we use the following protocol: a backend
 	 * increments st_changecount before modifying its entry, and again after
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index b552359915f..7ac2fb1512f 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -25,6 +25,7 @@
 #include "rewrite/prs2lock.h"
 #include "storage/block.h"
 #include "storage/relfilelocator.h"
+#include "storage/buf.h"
 #include "storage/smgr.h"
 #include "utils/relcache.h"
 #include "utils/reltrigger.h"
@@ -159,6 +160,9 @@ typedef struct RelationData
 
 	/* data managed by RelationGetIndexAttrBitmap: */
 	bool		rd_attrsvalid;	/* are bitmaps of attrs valid? */
+
+	Buffer		rd_recent_root;
+
 	Bitmapset  *rd_keyattr;		/* cols that can be ref'd by foreign keys */
 	Bitmapset  *rd_pkattr;		/* cols included in primary key */
 	Bitmapset  *rd_idattr;		/* included in replica identity index */
-- 
2.50.1