From bd7b2c4dd9ef4fba1f41b6842510f5818c16d7d9 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Tue, 27 Oct 2026 13:16:47 -0400 Subject: [PATCH v3_REL18 1/3] Don't register pin-only primary bucket pages in hash WAL records Replay of XLOG_HASH_DELETE, XLOG_HASH_MOVE_PAGE_CONTENTS and XLOG_HASH_SQUEEZE_PAGE must take a cleanup lock on the bucket's primary page, so that a hot standby scan cannot be in progress in the bucket, or start in it, while tuples are removed or moved. When the primary bucket page is not itself changed by the record, hashbucketcleanup(), _hash_squeezebucket() and _hash_freeovflpage() got that lock by registering the page with REGBUF_NO_CHANGE while holding only a pin on it. That breaks the rule that a registered buffer is content-locked and dirtied, and it is why wal_consistency_checking could compute the record CRC over a page that a concurrent inserter was still changing. Instead, store the primary bucket page's block number in the main data of these three records, and have replay read that block and take the cleanup lock on it directly, in the same order as before. The page is no longer registered unless the record changes it. For the squeeze record this also stops registering the write page when it is the primary bucket page and is not modified. With this, hash no longer uses REGBUF_NO_CHANGE. The three record formats change, so bump XLOG_PAGE_MAGIC. Discussion: https://postgr.es/m/055bb33c-bccc-bc1d-c2f8-8598534448ac@gmail.com --- src/backend/access/hash/README | 9 ++++ src/backend/access/hash/hash.c | 15 ++---- src/backend/access/hash/hash_xlog.c | 56 ++++++++++++++------ src/backend/access/hash/hashovfl.c | 71 ++++++++------------------ src/backend/access/rmgrdesc/hashdesc.c | 9 ++-- src/include/access/hash_xlog.h | 18 +++++-- 6 files changed, 94 insertions(+), 84 deletions(-) diff --git a/src/backend/access/hash/README b/src/backend/access/hash/README index fc9031117c9..6d9b14e2d21 100644 --- a/src/backend/access/hash/README +++ b/src/backend/access/hash/README @@ -641,6 +641,15 @@ entire bucket. After recovery, the operations will work correctly, but the index will remain bloated and this can impact performance of read and insert operations until the next vacuum squeeze the bucket completely. +Replay of the delete, move-page-contents and squeeze-page records takes a +cleanup lock on the primary bucket page before touching any other page, so +that no scan on a hot standby is in progress in the bucket or can start in +it while tuples are removed or moved; such a scan could otherwise miss +tuples or return them twice. When the primary bucket page is not one of +the pages the record changes, the operation holds only a pin on it, so it +is not registered in the record. The record stores its block number +instead, and replay locks that block directly. + Other Notes ----------- diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c index a7d7f2f6d32..d71fc75ea0c 100644 --- a/src/backend/access/hash/hash.c +++ b/src/backend/access/hash/hash.c @@ -823,6 +823,7 @@ hashbucketcleanup(Relation rel, Bucket cur_bucket, Buffer bucket_buf, xl_hash_delete xlrec; XLogRecPtr recptr; + xlrec.bucket_blkno = bucket_blkno; xlrec.clear_dead_marking = clear_dead_marking; xlrec.is_primary_bucket_page = (buf == bucket_buf); @@ -830,17 +831,11 @@ hashbucketcleanup(Relation rel, Bucket cur_bucket, Buffer bucket_buf, XLogRegisterData(&xlrec, SizeOfHashDelete); /* - * bucket buffer was not changed, but still needs to be - * registered to ensure that we can acquire a cleanup lock on - * it during replay. + * If buf is an overflow page, the primary bucket page is not + * changed and we hold only a pin on it, so it is not + * registered. Replay takes a cleanup lock on bucket_blkno + * instead. */ - if (!xlrec.is_primary_bucket_page) - { - uint8 flags = REGBUF_STANDARD | REGBUF_NO_IMAGE | REGBUF_NO_CHANGE; - - XLogRegisterBuffer(0, bucket_buf, flags); - } - XLogRegisterBuffer(1, buf, REGBUF_STANDARD); XLogRegisterBufData(1, deletable, ndeletable * sizeof(OffsetNumber)); diff --git a/src/backend/access/hash/hash_xlog.c b/src/backend/access/hash/hash_xlog.c index d4cb6246b48..b9ebdafae4c 100644 --- a/src/backend/access/hash/hash_xlog.c +++ b/src/backend/access/hash/hash_xlog.c @@ -471,6 +471,33 @@ hash_xlog_split_complete(XLogReaderState *record) UnlockReleaseBuffer(newbuf); } +/* + * Take a cleanup lock on the primary bucket page of the bucket that a + * squeeze or delete record operates on, when that page is not one of the + * pages the record changes and so is not registered in it. + * + * This serializes replay against scans on a hot standby, which pin the + * primary bucket page for their whole duration; see the comments in the + * callers. The page is not modified, so nothing is done with it besides + * locking it. block_id must be a block of the same index registered in + * the record. + */ +static Buffer +hash_xlog_lock_bucket(XLogReaderState *record, uint8 block_id, + BlockNumber bucket_blkno) +{ + RelFileLocator rlocator; + ForkNumber forknum; + Buffer buf; + + XLogRecGetBlockTag(record, block_id, &rlocator, &forknum, NULL); + buf = XLogReadBufferExtended(rlocator, forknum, bucket_blkno, + RBM_NORMAL, InvalidBuffer); + if (BufferIsValid(buf)) + LockBufferForCleanup(buf); + return buf; +} + /* * replay move of page contents for squeeze operation of hash index */ @@ -495,11 +522,7 @@ hash_xlog_move_page_contents(XLogReaderState *record) action = XLogReadBufferForRedoExtended(record, 1, RBM_NORMAL, true, &writebuf); else { - /* - * we don't care for return value as the purpose of reading bucketbuf - * is to ensure a cleanup lock on primary bucket page. - */ - (void) XLogReadBufferForRedoExtended(record, 0, RBM_NORMAL, true, &bucketbuf); + bucketbuf = hash_xlog_lock_bucket(record, 1, xldata->bucket_blkno); action = XLogReadBufferForRedo(record, 1, &writebuf); } @@ -620,14 +643,19 @@ hash_xlog_squeeze_page(XLogReaderState *record) * can miss some records or show the same record multiple times. */ if (xldata->is_prim_bucket_same_wrt) - action = XLogReadBufferForRedoExtended(record, 1, RBM_NORMAL, true, &writebuf); + { + /* block 1 is absent if the primary bucket page is not modified */ + if (XLogRecHasBlockRef(record, 1)) + action = XLogReadBufferForRedoExtended(record, 1, RBM_NORMAL, true, &writebuf); + else + { + bucketbuf = hash_xlog_lock_bucket(record, 2, xldata->bucket_blkno); + action = BLK_NOTFOUND; + } + } else { - /* - * we don't care for return value as the purpose of reading bucketbuf - * is to ensure a cleanup lock on primary bucket page. - */ - (void) XLogReadBufferForRedoExtended(record, 0, RBM_NORMAL, true, &bucketbuf); + bucketbuf = hash_xlog_lock_bucket(record, 2, xldata->bucket_blkno); if (xldata->ntups > 0 || xldata->is_prev_bucket_same_wrt) action = XLogReadBufferForRedo(record, 1, &writebuf); @@ -855,11 +883,7 @@ hash_xlog_delete(XLogReaderState *record) action = XLogReadBufferForRedoExtended(record, 1, RBM_NORMAL, true, &deletebuf); else { - /* - * we don't care for return value as the purpose of reading bucketbuf - * is to ensure a cleanup lock on primary bucket page. - */ - (void) XLogReadBufferForRedoExtended(record, 0, RBM_NORMAL, true, &bucketbuf); + bucketbuf = hash_xlog_lock_bucket(record, 1, xldata->bucket_blkno); action = XLogReadBufferForRedo(record, 1, &deletebuf); } diff --git a/src/backend/access/hash/hashovfl.c b/src/backend/access/hash/hashovfl.c index 4f5fd3b2837..3bec464efae 100644 --- a/src/backend/access/hash/hashovfl.c +++ b/src/backend/access/hash/hashovfl.c @@ -647,63 +647,37 @@ _hash_freeovflpage(Relation rel, Buffer bucketbuf, Buffer ovflbuf, xl_hash_squeeze_page xlrec; XLogRecPtr recptr; int i; - bool mod_wbuf = false; + bool mod_wbuf; + xlrec.bucket_blkno = BufferGetBlockNumber(bucketbuf); xlrec.prevblkno = prevblkno; xlrec.nextblkno = nextblkno; xlrec.ntups = nitups; xlrec.is_prim_bucket_same_wrt = (wbuf == bucketbuf); xlrec.is_prev_bucket_same_wrt = (wbuf == prevbuf); + /* wbuf is modified iff tuples are moved onto it or it is prevbuf */ + mod_wbuf = (nitups > 0 || xlrec.is_prev_bucket_same_wrt); + XLogBeginInsert(); XLogRegisterData(&xlrec, SizeOfHashSqueezePage); /* - * bucket buffer was not changed, but still needs to be registered to - * ensure that we can acquire a cleanup lock on it during replay. + * Register only the pages this record changes. The primary bucket + * page is registered only if it is the write page and is modified; + * otherwise we may hold only a pin on it, and replay takes a cleanup + * lock on bucket_blkno instead. */ - if (!xlrec.is_prim_bucket_same_wrt) - { - uint8 flags = REGBUF_STANDARD | REGBUF_NO_IMAGE | REGBUF_NO_CHANGE; - - XLogRegisterBuffer(0, bucketbuf, flags); - } - - if (xlrec.ntups > 0) + if (mod_wbuf) { XLogRegisterBuffer(1, wbuf, REGBUF_STANDARD); - - /* Remember that wbuf is modified. */ - mod_wbuf = true; - - XLogRegisterBufData(1, itup_offsets, - nitups * sizeof(OffsetNumber)); - for (i = 0; i < nitups; i++) - XLogRegisterBufData(1, itups[i], tups_size[i]); - } - else if (xlrec.is_prim_bucket_same_wrt || xlrec.is_prev_bucket_same_wrt) - { - uint8 wbuf_flags; - - /* - * A write buffer needs to be registered even if no tuples are - * added to it to ensure that we can acquire a cleanup lock on it - * if it is the same as primary bucket buffer or update the - * nextblkno if it is same as the previous bucket buffer. - */ - Assert(xlrec.ntups == 0); - - wbuf_flags = REGBUF_STANDARD; - if (!xlrec.is_prev_bucket_same_wrt) - { - wbuf_flags |= REGBUF_NO_CHANGE; - } - else + if (nitups > 0) { - /* Remember that wbuf is modified. */ - mod_wbuf = true; + XLogRegisterBufData(1, itup_offsets, + nitups * sizeof(OffsetNumber)); + for (i = 0; i < nitups; i++) + XLogRegisterBufData(1, itups[i], tups_size[i]); } - XLogRegisterBuffer(1, wbuf, wbuf_flags); } XLogRegisterBuffer(2, ovflbuf, REGBUF_STANDARD); @@ -989,6 +963,7 @@ readpage: XLogRecPtr recptr; xl_hash_move_page_contents xlrec; + xlrec.bucket_blkno = bucket_blkno; xlrec.ntups = nitups; xlrec.is_prim_bucket_same_wrt = (wbuf == bucket_buf); @@ -996,17 +971,11 @@ readpage: XLogRegisterData(&xlrec, SizeOfHashMovePageContents); /* - * bucket buffer was not changed, but still needs to - * be registered to ensure that we can acquire a - * cleanup lock on it during replay. + * If wbuf is an overflow page, the primary bucket + * page is not changed and we hold only a pin on it, + * so it is not registered. Replay takes a cleanup + * lock on bucket_blkno instead. */ - if (!xlrec.is_prim_bucket_same_wrt) - { - int flags = REGBUF_STANDARD | REGBUF_NO_IMAGE | REGBUF_NO_CHANGE; - - XLogRegisterBuffer(0, bucket_buf, flags); - } - XLogRegisterBuffer(1, wbuf, REGBUF_STANDARD); XLogRegisterBufData(1, itup_offsets, nitups * sizeof(OffsetNumber)); diff --git a/src/backend/access/rmgrdesc/hashdesc.c b/src/backend/access/rmgrdesc/hashdesc.c index 75f43a91520..8872c521e69 100644 --- a/src/backend/access/rmgrdesc/hashdesc.c +++ b/src/backend/access/rmgrdesc/hashdesc.c @@ -76,7 +76,8 @@ hash_desc(StringInfo buf, XLogReaderState *record) { xl_hash_move_page_contents *xlrec = (xl_hash_move_page_contents *) rec; - appendStringInfo(buf, "ntups %d, is_primary %c", + appendStringInfo(buf, "bucket_blkno %u, ntups %d, is_primary %c", + xlrec->bucket_blkno, xlrec->ntups, xlrec->is_prim_bucket_same_wrt ? 'T' : 'F'); break; @@ -85,7 +86,8 @@ hash_desc(StringInfo buf, XLogReaderState *record) { xl_hash_squeeze_page *xlrec = (xl_hash_squeeze_page *) rec; - appendStringInfo(buf, "prevblkno %u, nextblkno %u, ntups %d, is_primary %c", + appendStringInfo(buf, "bucket_blkno %u, prevblkno %u, nextblkno %u, ntups %d, is_primary %c", + xlrec->bucket_blkno, xlrec->prevblkno, xlrec->nextblkno, xlrec->ntups, @@ -96,7 +98,8 @@ hash_desc(StringInfo buf, XLogReaderState *record) { xl_hash_delete *xlrec = (xl_hash_delete *) rec; - appendStringInfo(buf, "clear_dead_marking %c, is_primary %c", + appendStringInfo(buf, "bucket_blkno %u, clear_dead_marking %c, is_primary %c", + xlrec->bucket_blkno, xlrec->clear_dead_marking ? 'T' : 'F', xlrec->is_primary_bucket_page ? 'T' : 'F'); break; diff --git a/src/include/access/hash_xlog.h b/src/include/access/hash_xlog.h index 5d4671dc4c1..8580d5dbead 100644 --- a/src/include/access/hash_xlog.h +++ b/src/include/access/hash_xlog.h @@ -129,12 +129,15 @@ typedef struct xl_hash_split_complete * * This data record is used for XLOG_HASH_MOVE_PAGE_CONTENTS * - * Backup Blk 0: primary bucket page * Backup Blk 1: page containing moved tuples * Backup Blk 2: page from which tuples will be removed + * + * The primary bucket page is not registered; replay takes a cleanup lock on + * bucket_blkno instead. */ typedef struct xl_hash_move_page_contents { + BlockNumber bucket_blkno; /* primary bucket page */ uint16 ntups; bool is_prim_bucket_same_wrt; /* true if the page to which * tuples are moved is same as @@ -149,16 +152,20 @@ typedef struct xl_hash_move_page_contents * * This data record is used for XLOG_HASH_SQUEEZE_PAGE * - * Backup Blk 0: primary bucket page - * Backup Blk 1: page containing tuples moved from freed overflow page + * Backup Blk 1: page containing tuples moved from freed overflow page, if + * it is modified * Backup Blk 2: freed overflow page * Backup Blk 3: page previous to the freed overflow page * Backup Blk 4: page next to the freed overflow page * Backup Blk 5: bitmap page containing info of freed overflow page * Backup Blk 6: meta page + * + * The primary bucket page is registered only as block 1, when it is the + * modified write page; otherwise replay takes a cleanup lock on bucket_blkno. */ typedef struct xl_hash_squeeze_page { + BlockNumber bucket_blkno; /* primary bucket page */ BlockNumber prevblkno; BlockNumber nextblkno; uint16 ntups; @@ -179,11 +186,14 @@ typedef struct xl_hash_squeeze_page * * This data record is used for XLOG_HASH_DELETE * - * Backup Blk 0: primary bucket page * Backup Blk 1: page from which tuples are deleted + * + * If block 1 is not the primary bucket page, replay takes a cleanup lock on + * bucket_blkno, which is not registered. */ typedef struct xl_hash_delete { + BlockNumber bucket_blkno; /* primary bucket page */ bool clear_dead_marking; /* true if this operation clears * LH_PAGE_HAS_DEAD_TUPLES flag */ bool is_primary_bucket_page; /* true if the operation is for -- 2.55.0