From dc023aa6ece08eba610e5b7a0a69f82c62113be0 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Mon, 19 May 2025 13:05:28 -0400 Subject: [PATCH v1] Avoid BufferGetLSNAtomic() calls during index-only scans. --- src/include/access/nbtree.h | 1 + src/backend/access/nbtree/nbtpreprocesskeys.c | 8 +++++++ src/backend/access/nbtree/nbtsearch.c | 22 ++++++++++--------- src/backend/access/nbtree/nbtutils.c | 11 +++++----- 4 files changed, 26 insertions(+), 16 deletions(-) diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h index ebca02588..933507ec3 100644 --- a/src/include/access/nbtree.h +++ b/src/include/access/nbtree.h @@ -1054,6 +1054,7 @@ typedef struct BTScanOpaqueData { /* these fields are set by _bt_preprocess_keys(): */ bool qual_ok; /* false if qual can never be satisfied */ + bool drop_pin; /* true if it's safe to drop leaf page pins */ int numberOfKeys; /* number of preprocessed scan keys */ ScanKey keyData; /* array of preprocessed scan keys */ diff --git a/src/backend/access/nbtree/nbtpreprocesskeys.c b/src/backend/access/nbtree/nbtpreprocesskeys.c index a136e4bbf..b38fe013a 100644 --- a/src/backend/access/nbtree/nbtpreprocesskeys.c +++ b/src/backend/access/nbtree/nbtpreprocesskeys.c @@ -205,6 +205,14 @@ _bt_preprocess_keys(IndexScanDesc scan) /* initialize result variables */ so->qual_ok = true; + + /* + * Can only safely drop leaf page pins during plain index scans of logged + * relations (XXX What about bitmap index scans?) + */ + so->drop_pin = (IsMVCCSnapshot(scan->xs_snapshot) && + RelationNeedsWAL(scan->indexRelation) && + !scan->xs_want_itup); so->numberOfKeys = 0; if (numberOfKeys < 1) diff --git a/src/backend/access/nbtree/nbtsearch.c b/src/backend/access/nbtree/nbtsearch.c index fe9a38869..7d6361949 100644 --- a/src/backend/access/nbtree/nbtsearch.c +++ b/src/backend/access/nbtree/nbtsearch.c @@ -25,7 +25,8 @@ #include "utils/rel.h" -static void _bt_drop_lock_and_maybe_pin(IndexScanDesc scan, BTScanPos sp); +static inline void _bt_drop_lock_and_maybe_pin(Relation rel, BTScanOpaque so, + BTScanPos sp); static Buffer _bt_moveright(Relation rel, Relation heaprel, BTScanInsert key, Buffer buf, bool forupdate, BTStack stack, int access); @@ -63,14 +64,12 @@ static bool _bt_endpoint(IndexScanDesc scan, ScanDirection dir); * * See nbtree/README section on making concurrent TID recycling safe. */ -static void -_bt_drop_lock_and_maybe_pin(IndexScanDesc scan, BTScanPos sp) +static inline void +_bt_drop_lock_and_maybe_pin(Relation rel, BTScanOpaque so, BTScanPos sp) { - _bt_unlockbuf(scan->indexRelation, sp->buf); + _bt_unlockbuf(rel, sp->buf); - if (IsMVCCSnapshot(scan->xs_snapshot) && - RelationNeedsWAL(scan->indexRelation) && - !scan->xs_want_itup) + if (so->drop_pin) { ReleaseBuffer(sp->buf); sp->buf = InvalidBuffer; @@ -1627,7 +1626,8 @@ _bt_readpage(IndexScanDesc scan, ScanDirection dir, OffsetNumber offnum, } /* initialize remaining currPos fields related to current page */ - so->currPos.lsn = BufferGetLSNAtomic(so->currPos.buf); + if (so->drop_pin) + so->currPos.lsn = BufferGetLSNAtomic(so->currPos.buf); so->currPos.dir = dir; so->currPos.nextTupleOffset = 0; /* either moreLeft or moreRight should be set now (may be unset later) */ @@ -2251,12 +2251,14 @@ _bt_readfirstpage(IndexScanDesc scan, OffsetNumber offnum, ScanDirection dir) */ if (_bt_readpage(scan, dir, offnum, true)) { + Relation rel = scan->indexRelation; + /* * _bt_readpage succeeded. Drop the lock (and maybe the pin) on * so->currPos.buf in preparation for btgettuple returning tuples. */ Assert(BTScanPosIsPinned(so->currPos)); - _bt_drop_lock_and_maybe_pin(scan, &so->currPos); + _bt_drop_lock_and_maybe_pin(rel, so, &so->currPos); return true; } @@ -2413,7 +2415,7 @@ _bt_readnextpage(IndexScanDesc scan, BlockNumber blkno, */ Assert(so->currPos.currPage == blkno); Assert(BTScanPosIsPinned(so->currPos)); - _bt_drop_lock_and_maybe_pin(scan, &so->currPos); + _bt_drop_lock_and_maybe_pin(rel, so, &so->currPos); return true; } diff --git a/src/backend/access/nbtree/nbtutils.c b/src/backend/access/nbtree/nbtutils.c index 1a15dfcb7..7f72a1a77 100644 --- a/src/backend/access/nbtree/nbtutils.c +++ b/src/backend/access/nbtree/nbtutils.c @@ -3364,7 +3364,6 @@ _bt_killitems(IndexScanDesc scan) int i; int numKilled = so->numKilled; bool killedsomething = false; - bool droppedpin PG_USED_FOR_ASSERTS_ONLY; Assert(BTScanPosIsValid(so->currPos)); @@ -3374,7 +3373,7 @@ _bt_killitems(IndexScanDesc scan) */ so->numKilled = 0; - if (BTScanPosIsPinned(so->currPos)) + if (!so->drop_pin) { /* * We have held the pin on this page since we read the index tuples, @@ -3382,7 +3381,7 @@ _bt_killitems(IndexScanDesc scan) * re-use of any TID on the page, so there is no need to check the * LSN. */ - droppedpin = false; + Assert(BTScanPosIsPinned(so->currPos)); _bt_lockbuf(scan->indexRelation, so->currPos.buf, BT_READ); page = BufferGetPage(so->currPos.buf); @@ -3391,8 +3390,8 @@ _bt_killitems(IndexScanDesc scan) { Buffer buf; - droppedpin = true; /* Attempt to re-read the buffer, getting pin and lock. */ + Assert(!BTScanPosIsPinned(so->currPos)); buf = _bt_getbuf(scan->indexRelation, so->currPos.currPage, BT_READ); page = BufferGetPage(buf); @@ -3442,7 +3441,7 @@ _bt_killitems(IndexScanDesc scan) * correctness. * * Note that the page may have been modified in almost any way - * since we first read it (in the !droppedpin case), so it's + * since we first read it (in the !so->drop_pin case), so it's * possible that this posting list tuple wasn't a posting list * tuple when we first encountered its heap TIDs. */ @@ -3458,7 +3457,7 @@ _bt_killitems(IndexScanDesc scan) * though only in the common case where the page can't * have been concurrently modified */ - Assert(kitem->indexOffset == offnum || !droppedpin); + Assert(kitem->indexOffset == offnum || !so->drop_pin); /* * Read-ahead to later kitems here. -- 2.49.0