From 9f399e7c90fd3ac14a0fed2697f79fa77c264367 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Thu, 29 Oct 2026 10:30:14 -0400 Subject: [PATCH v5 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 primary bucket page so that no scan on a hot standby is in progress in the bucket, and none can start, while tuples are removed or moved. That lock was obtained by registering the page (pin-only) with REGBUF_NO_CHANGE; redo never reads data from the block, the registration existed only for the lock. That broke the rule that a registered buffer is one the caller holds exclusively locked and dirty, and under wal_consistency_checking it forced a full-page image of a page a concurrent inserter could still modify, so the record CRC was computed over shifting data. Stop registering the primary bucket page when the record does not change it. When the page is not one of the pages the record changes, the operation holds only a pin on it, so the record carries the page's block number in its main data and replay reads that block and takes a cleanup lock on it directly, at the same point and in the same order as before. The block number is appended only in that case, so the records are no larger than before in the common case where the primary bucket page is registered. After this change hash registers only buffers it holds exclusively locked and dirty, and there are no REGBUF_NO_CHANGE users left. The three record formats change, so bump XLOG_PAGE_MAGIC; master only. Co-authored-by: Vignesh C Suggested-by: Amit Kapila Discussion: https://postgr.es/m/auNs8b_vS2OPdTL8@floki --- src/backend/access/hash/README | 9 +++ src/backend/access/hash/hash.c | 17 ++--- src/backend/access/hash/hash_xlog.c | 99 +++++++++++++++++---------- src/backend/access/hash/hashovfl.c | 100 ++++++++++++---------------- src/include/access/hash_xlog.h | 37 ++++++---- src/include/access/xlog_internal.h | 2 +- 6 files changed, 149 insertions(+), 115 deletions(-) diff --git a/src/backend/access/hash/README b/src/backend/access/hash/README index fc9031117c9..7774e1d2fe0 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 then carries 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..7a79030303e 100644 --- a/src/backend/access/hash/hash.c +++ b/src/backend/access/hash/hash.c @@ -909,19 +909,16 @@ 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. Append its block number instead, and replay + * takes a cleanup lock on it. It is not needed otherwise. */ if (!xlrec.is_primary_bucket_page) - { - uint8 flags = REGBUF_STANDARD | REGBUF_NO_IMAGE | REGBUF_NO_CHANGE; - - XLogRegisterBuffer(0, bucket_buf, flags); - } + XLogRegisterData(&bucket_blkno, sizeof(BlockNumber)); - XLogRegisterBuffer(1, buf, REGBUF_STANDARD); - XLogRegisterBufData(1, deletable, + XLogRegisterBuffer(0, buf, REGBUF_STANDARD); + XLogRegisterBufData(0, deletable, ndeletable * sizeof(OffsetNumber)); recptr = XLogInsert(RM_HASH_ID, XLOG_HASH_DELETE); diff --git a/src/backend/access/hash/hash_xlog.c b/src/backend/access/hash/hash_xlog.c index e9a2b9aa9a7..e18bb94b89f 100644 --- a/src/backend/access/hash/hash_xlog.c +++ b/src/backend/access/hash/hash_xlog.c @@ -469,6 +469,38 @@ 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. Its block number + * is stored in the main data of the record, right after the fixed-size struct + * of rec_size bytes. + * + * 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, Size rec_size) +{ + RelFileLocator rlocator; + ForkNumber forknum; + BlockNumber bucket_blkno; + Buffer buf; + + Assert(XLogRecGetDataLen(record) == rec_size + sizeof(BlockNumber)); + memcpy(&bucket_blkno, XLogRecGetData(record) + rec_size, sizeof(BlockNumber)); + + 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 */ @@ -490,16 +522,12 @@ hash_xlog_move_page_contents(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); + action = XLogReadBufferForRedoExtended(record, 0, 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, 0, SizeOfHashMovePageContents); - action = XLogReadBufferForRedo(record, 1, &writebuf); + action = XLogReadBufferForRedo(record, 0, &writebuf); } /* replay the record for adding entries in overflow buffer */ @@ -511,7 +539,7 @@ hash_xlog_move_page_contents(XLogReaderState *record) Size datalen; uint16 ninserted = 0; - data = begin = XLogRecGetBlockData(record, 1, &datalen); + data = begin = XLogRecGetBlockData(record, 0, &datalen); writepage = BufferGetPage(writebuf); @@ -550,13 +578,13 @@ hash_xlog_move_page_contents(XLogReaderState *record) } /* replay the record for deleting entries from overflow buffer */ - if (XLogReadBufferForRedo(record, 2, &deletebuf) == BLK_NEEDS_REDO) + if (XLogReadBufferForRedo(record, 1, &deletebuf) == BLK_NEEDS_REDO) { Page page; char *ptr; Size len; - ptr = XLogRecGetBlockData(record, 2, &len); + ptr = XLogRecGetBlockData(record, 1, &len); page = BufferGetPage(deletebuf); @@ -617,17 +645,22 @@ 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 0 is absent if the primary bucket page is not modified */ + if (XLogRecHasBlockRef(record, 0)) + action = XLogReadBufferForRedoExtended(record, 0, RBM_NORMAL, true, &writebuf); + else + { + bucketbuf = hash_xlog_lock_bucket(record, 1, SizeOfHashSqueezePage); + 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, 1, SizeOfHashSqueezePage); if (xldata->ntups > 0 || xldata->is_prev_bucket_same_wrt) - action = XLogReadBufferForRedo(record, 1, &writebuf); + action = XLogReadBufferForRedo(record, 0, &writebuf); else action = BLK_NOTFOUND; } @@ -642,7 +675,7 @@ hash_xlog_squeeze_page(XLogReaderState *record) uint16 ninserted = 0; bool mod_wbuf = false; - data = begin = XLogRecGetBlockData(record, 1, &datalen); + data = begin = XLogRecGetBlockData(record, 0, &datalen); writepage = BufferGetPage(writebuf); @@ -708,7 +741,7 @@ hash_xlog_squeeze_page(XLogReaderState *record) } /* replay the record for initializing overflow buffer */ - if (XLogReadBufferForRedo(record, 2, &ovflbuf) == BLK_NEEDS_REDO) + if (XLogReadBufferForRedo(record, 1, &ovflbuf) == BLK_NEEDS_REDO) { Page ovflpage; HashPageOpaque ovflopaque; @@ -733,7 +766,7 @@ hash_xlog_squeeze_page(XLogReaderState *record) /* replay the record for page previous to the freed overflow page */ if (!xldata->is_prev_bucket_same_wrt && - XLogReadBufferForRedo(record, 3, &prevbuf) == BLK_NEEDS_REDO) + XLogReadBufferForRedo(record, 2, &prevbuf) == BLK_NEEDS_REDO) { Page prevpage = BufferGetPage(prevbuf); HashPageOpaque prevopaque = HashPageGetOpaque(prevpage); @@ -747,11 +780,11 @@ hash_xlog_squeeze_page(XLogReaderState *record) UnlockReleaseBuffer(prevbuf); /* replay the record for page next to the freed overflow page */ - if (XLogRecHasBlockRef(record, 4)) + if (XLogRecHasBlockRef(record, 3)) { Buffer nextbuf; - if (XLogReadBufferForRedo(record, 4, &nextbuf) == BLK_NEEDS_REDO) + if (XLogReadBufferForRedo(record, 3, &nextbuf) == BLK_NEEDS_REDO) { Page nextpage = BufferGetPage(nextbuf); HashPageOpaque nextopaque = HashPageGetOpaque(nextpage); @@ -778,7 +811,7 @@ hash_xlog_squeeze_page(XLogReaderState *record) * index updates can be happening concurrently. */ /* replay the record for bitmap page */ - if (XLogReadBufferForRedo(record, 5, &mapbuf) == BLK_NEEDS_REDO) + if (XLogReadBufferForRedo(record, 4, &mapbuf) == BLK_NEEDS_REDO) { Page mappage = BufferGetPage(mapbuf); uint32 *freep = NULL; @@ -788,7 +821,7 @@ hash_xlog_squeeze_page(XLogReaderState *record) freep = HashPageGetBitmap(mappage); - data = XLogRecGetBlockData(record, 5, &datalen); + data = XLogRecGetBlockData(record, 4, &datalen); bitmap_page_bit = (uint32 *) data; CLRBIT(freep, *bitmap_page_bit); @@ -800,11 +833,11 @@ hash_xlog_squeeze_page(XLogReaderState *record) UnlockReleaseBuffer(mapbuf); /* replay the record for meta page */ - if (XLogRecHasBlockRef(record, 6)) + if (XLogRecHasBlockRef(record, 5)) { Buffer metabuf; - if (XLogReadBufferForRedo(record, 6, &metabuf) == BLK_NEEDS_REDO) + if (XLogReadBufferForRedo(record, 5, &metabuf) == BLK_NEEDS_REDO) { HashMetaPage metap; Page page; @@ -812,7 +845,7 @@ hash_xlog_squeeze_page(XLogReaderState *record) uint32 *firstfree_ovflpage; Size datalen; - data = XLogRecGetBlockData(record, 6, &datalen); + data = XLogRecGetBlockData(record, 5, &datalen); firstfree_ovflpage = (uint32 *) data; page = BufferGetPage(metabuf); @@ -848,16 +881,12 @@ hash_xlog_delete(XLogReaderState *record) * can miss some records or show the same record multiple times. */ if (xldata->is_primary_bucket_page) - action = XLogReadBufferForRedoExtended(record, 1, RBM_NORMAL, true, &deletebuf); + action = XLogReadBufferForRedoExtended(record, 0, 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, 0, SizeOfHashDelete); - action = XLogReadBufferForRedo(record, 1, &deletebuf); + action = XLogReadBufferForRedo(record, 0, &deletebuf); } /* replay the record for deleting entries in bucket page */ @@ -866,7 +895,7 @@ hash_xlog_delete(XLogReaderState *record) char *ptr; Size len; - ptr = XLogRecGetBlockData(record, 1, &len); + ptr = XLogRecGetBlockData(record, 0, &len); page = BufferGetPage(deletebuf); diff --git a/src/backend/access/hash/hashovfl.c b/src/backend/access/hash/hashovfl.c index dbc57ef958c..7ad934ca38f 100644 --- a/src/backend/access/hash/hashovfl.c +++ b/src/backend/access/hash/hashovfl.c @@ -580,9 +580,13 @@ _hash_freeovflpage(Relation rel, Buffer bucketbuf, Buffer ovflbuf, /* Get write-lock on metapage to update firstfree */ LockBuffer(metabuf, BUFFER_LOCK_EXCLUSIVE); - /* This operation needs to log multiple tuples, prepare WAL for that */ + /* + * This operation needs to log multiple tuples, prepare WAL for that. One + * extra data segment accounts for the primary bucket page's block number, + * appended to the record when that page is pin-only (not registered). + */ if (RelationNeedsWAL(rel)) - XLogEnsureRecordSpace(HASH_XLOG_FREE_OVFL_BUFS, 4 + nitups); + XLogEnsureRecordSpace(HASH_XLOG_FREE_OVFL_BUFS, 5 + nitups); START_CRIT_SECTION(); @@ -656,6 +660,7 @@ _hash_freeovflpage(Relation rel, Buffer bucketbuf, Buffer ovflbuf, if (RelationNeedsWAL(rel)) { xl_hash_squeeze_page xlrec; + BlockNumber bucket_blkno = BufferGetBlockNumber(bucketbuf); xlrec.prevblkno = prevblkno; xlrec.nextblkno = nextblkno; @@ -667,45 +672,27 @@ _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, so append its block number + * and replay takes a cleanup lock on it. */ - if (!is_prim_bucket_same_wrt) - { - uint8 flags = REGBUF_STANDARD | REGBUF_NO_IMAGE | REGBUF_NO_CHANGE; - - XLogRegisterBuffer(0, bucketbuf, flags); - } + if (!(is_prim_bucket_same_wrt && mod_wbuf)) + XLogRegisterData(&bucket_blkno, sizeof(BlockNumber)); - if (nitups > 0) - { - 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) + if (mod_wbuf) { - 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); + XLogRegisterBuffer(0, wbuf, REGBUF_STANDARD); + if (nitups > 0) + { + XLogRegisterBufData(0, itup_offsets, + nitups * sizeof(OffsetNumber)); + for (int i = 0; i < nitups; i++) + XLogRegisterBufData(0, itups[i], tups_size[i]); + } } - XLogRegisterBuffer(2, ovflbuf, REGBUF_STANDARD); + XLogRegisterBuffer(1, ovflbuf, REGBUF_STANDARD); /* * If prevpage and the writepage (block in which we are moving tuples @@ -714,18 +701,18 @@ _hash_freeovflpage(Relation rel, Buffer bucketbuf, Buffer ovflbuf, * writepage. */ if (BufferIsValid(prevbuf) && !is_prev_bucket_same_wrt) - XLogRegisterBuffer(3, prevbuf, REGBUF_STANDARD); + XLogRegisterBuffer(2, prevbuf, REGBUF_STANDARD); if (BufferIsValid(nextbuf)) - XLogRegisterBuffer(4, nextbuf, REGBUF_STANDARD); + XLogRegisterBuffer(3, nextbuf, REGBUF_STANDARD); - XLogRegisterBuffer(5, mapbuf, REGBUF_STANDARD); - XLogRegisterBufData(5, &bitmapbit, sizeof(uint32)); + XLogRegisterBuffer(4, mapbuf, REGBUF_STANDARD); + XLogRegisterBufData(4, &bitmapbit, sizeof(uint32)); if (update_metap) { - XLogRegisterBuffer(6, metabuf, REGBUF_STANDARD); - XLogRegisterBufData(6, &metap->hashm_firstfree, sizeof(uint32)); + XLogRegisterBuffer(5, metabuf, REGBUF_STANDARD); + XLogRegisterBufData(5, &metap->hashm_firstfree, sizeof(uint32)); } recptr = XLogInsert(RM_HASH_ID, XLOG_HASH_SQUEEZE_PAGE); @@ -966,10 +953,12 @@ readpage: /* * This operation needs to log multiple tuples, prepare - * WAL for that. + * WAL for that. One extra data segment accounts for the + * primary bucket page's block number, appended to the + * record when that page is pin-only (not registered). */ if (RelationNeedsWAL(rel)) - XLogEnsureRecordSpace(0, 3 + nitups); + XLogEnsureRecordSpace(0, 4 + nitups); START_CRIT_SECTION(); @@ -998,25 +987,22 @@ 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. Append its block number + * instead, and replay takes a cleanup lock on it. */ if (!xlrec.is_prim_bucket_same_wrt) - { - int flags = REGBUF_STANDARD | REGBUF_NO_IMAGE | REGBUF_NO_CHANGE; - - XLogRegisterBuffer(0, bucket_buf, flags); - } + XLogRegisterData(&bucket_blkno, sizeof(BlockNumber)); - XLogRegisterBuffer(1, wbuf, REGBUF_STANDARD); - XLogRegisterBufData(1, itup_offsets, + XLogRegisterBuffer(0, wbuf, REGBUF_STANDARD); + XLogRegisterBufData(0, itup_offsets, nitups * sizeof(OffsetNumber)); for (i = 0; i < nitups; i++) - XLogRegisterBufData(1, itups[i], tups_size[i]); + XLogRegisterBufData(0, itups[i], tups_size[i]); - XLogRegisterBuffer(2, rbuf, REGBUF_STANDARD); - XLogRegisterBufData(2, deletable, + XLogRegisterBuffer(1, rbuf, REGBUF_STANDARD); + XLogRegisterBufData(1, deletable, ndeletable * sizeof(OffsetNumber)); recptr = XLogInsert(RM_HASH_ID, XLOG_HASH_MOVE_PAGE_CONTENTS); diff --git a/src/include/access/hash_xlog.h b/src/include/access/hash_xlog.h index 149aed69dc5..0bb0d0590f5 100644 --- a/src/include/access/hash_xlog.h +++ b/src/include/access/hash_xlog.h @@ -129,9 +129,13 @@ 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 + * Backup Blk 0: page containing moved tuples + * Backup Blk 1: page from which tuples will be removed + * + * The primary bucket page is not registered. If it is not the page moved to + * (is_prim_bucket_same_wrt is false), the block number of the primary bucket + * page follows the struct as a BlockNumber, and replay takes a cleanup lock + * on that block. */ typedef struct xl_hash_move_page_contents { @@ -149,13 +153,18 @@ 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 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 + * Backup Blk 0: page containing tuples moved from freed overflow page, if + * it is modified + * Backup Blk 1: freed overflow page + * Backup Blk 2: page previous to the freed overflow page + * Backup Blk 3: page next to the freed overflow page + * Backup Blk 4: bitmap page containing info of freed overflow page + * Backup Blk 5: meta page + * + * The primary bucket page is registered only as block 0, when it is the + * modified write page. Otherwise the block number of the primary bucket + * page follows the struct as a BlockNumber, and replay takes a cleanup lock + * on that block. */ typedef struct xl_hash_squeeze_page { @@ -179,8 +188,12 @@ 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 + * Backup Blk 0: page from which tuples are deleted + * + * If block 0 is not the primary bucket page (is_primary_bucket_page is + * false), the block number of the primary bucket page follows the struct as a + * BlockNumber, and replay takes a cleanup lock on that block. The primary + * bucket page is not registered. */ typedef struct xl_hash_delete { 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.54.0