From 730467cfcf1d1b7f0a22f61bd48a37371f80cce9 Mon Sep 17 00:00:00 2001 From: Andres Freund 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