From 879be01737a6c7d5003d1c20ba4d803918c30221 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Tue, 27 Oct 2026 13:16:47 -0400 Subject: [PATCH v1 1/2] 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 | 61 ++++++++------------------ src/backend/access/rmgrdesc/hashdesc.c | 9 ++-- src/include/access/hash_xlog.h | 18 ++++++-- src/include/access/xlog_internal.h | 2 +- 7 files changed, 93 insertions(+), 77 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 63846c10840..fc93d78f6bb 100644 --- a/src/backend/access/hash/hash.c +++ b/src/backend/access/hash/hash.c @@ -902,6 +902,7 @@ hashbucketcleanup(Relation rel, Bucket cur_bucket, Buffer bucket_buf, { xl_hash_delete xlrec; + xlrec.bucket_blkno = bucket_blkno; xlrec.clear_dead_marking = clear_dead_marking; xlrec.is_primary_bucket_page = (buf == bucket_buf); @@ -909,17 +910,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 e9a2b9aa9a7..04eb8e17dd4 100644 --- a/src/backend/access/hash/hash_xlog.c +++ b/src/backend/access/hash/hash_xlog.c @@ -469,6 +469,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 */ @@ -493,11 +520,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); } @@ -617,14 +640,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); @@ -851,11 +879,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 dbc57ef958c..2bc9015c223 100644 --- a/src/backend/access/hash/hashovfl.c +++ b/src/backend/access/hash/hashovfl.c @@ -657,6 +657,7 @@ _hash_freeovflpage(Relation rel, Buffer bucketbuf, Buffer ovflbuf, { xl_hash_squeeze_page xlrec; + xlrec.bucket_blkno = BufferGetBlockNumber(bucketbuf); xlrec.prevblkno = prevblkno; xlrec.nextblkno = nextblkno; xlrec.ntups = nitups; @@ -667,42 +668,21 @@ _hash_freeovflpage(Relation rel, Buffer bucketbuf, Buffer ovflbuf, 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 (!is_prim_bucket_same_wrt) - { - uint8 flags = REGBUF_STANDARD | REGBUF_NO_IMAGE | REGBUF_NO_CHANGE; - - XLogRegisterBuffer(0, bucketbuf, flags); - } - - if (nitups > 0) + if (mod_wbuf) { XLogRegisterBuffer(1, wbuf, REGBUF_STANDARD); - XLogRegisterBufData(1, itup_offsets, - nitups * sizeof(OffsetNumber)); - for (int i = 0; i < nitups; i++) - XLogRegisterBufData(1, itups[i], tups_size[i]); - } - else if (is_prim_bucket_same_wrt || 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(nitups == 0); - - wbuf_flags = REGBUF_STANDARD; - if (!is_prev_bucket_same_wrt) - wbuf_flags |= REGBUF_NO_CHANGE; - else - Assert(mod_wbuf); - XLogRegisterBuffer(1, wbuf, wbuf_flags); + if (nitups > 0) + { + XLogRegisterBufData(1, itup_offsets, + nitups * sizeof(OffsetNumber)); + for (int i = 0; i < nitups; i++) + XLogRegisterBufData(1, itups[i], tups_size[i]); + } } XLogRegisterBuffer(2, ovflbuf, REGBUF_STANDARD); @@ -991,6 +971,7 @@ readpage: { xl_hash_move_page_contents xlrec; + xlrec.bucket_blkno = bucket_blkno; xlrec.ntups = nitups; xlrec.is_prim_bucket_same_wrt = (wbuf == bucket_buf); @@ -998,17 +979,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 593013afe9c..6988b6a82c3 100644 --- a/src/backend/access/rmgrdesc/hashdesc.c +++ b/src/backend/access/rmgrdesc/hashdesc.c @@ -80,7 +80,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; @@ -89,7 +90,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, is_previous %c", + appendStringInfo(buf, "bucket_blkno %u, prevblkno %u, nextblkno %u, ntups %d, is_primary %c, is_previous %c", + xlrec->bucket_blkno, xlrec->prevblkno, xlrec->nextblkno, xlrec->ntups, @@ -101,7 +103,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 149aed69dc5..9d762ff70d3 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 diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h index bf609c3c703..ed8f488dda5 100644 --- a/src/include/access/xlog_internal.h +++ b/src/include/access/xlog_internal.h @@ -32,7 +32,7 @@ /* * Each page of XLOG file has a header like this: */ -#define XLOG_PAGE_MAGIC 0xD122 /* can be used as WAL version indicator */ +#define XLOG_PAGE_MAGIC 0xD123 /* can be used as WAL version indicator */ typedef struct XLogPageHeaderData { -- 2.50.1