From 859c8f4284de1030ab070630fa06af3c171264d9 Mon Sep 17 00:00:00 2001 From: Jeff Davis Date: Thu, 28 Sep 2023 11:19:06 -0700 Subject: [PATCH v3] Assert that buffers are marked dirty before XLogRegisterBuffer(). A page must be marked dirty before writing WAL, as documented in transam/README. Enforce this rule in XLogRegisterBuffer(), though it's not actually a bug if (a) the page really is clean; or (b) the buffer is marked dirty after XLogRegisterBuffer() and before XLogInsert(). Update log_newpage_range(), where it's trivial to change the order so that we can check in XLogRegisterBuffer(). Other callers are more complex, and updating them is outside the scope of this patch (and perhaps not desirable), so introduce a flag REGBUF_CLEAN_OK to bypass the Assert. Note that this commit may have missed some callsites which can, at least in some cases, register a clean buffer. If such a callsite is found, it can be updated to use REGBUF_CLEAN_OK assuming it doesn't represent an actual bug. Discussion: https://postgr.es/m/c84114f8-c7f1-5b57-f85a-3adc31e1a904@iki.fi --- src/backend/access/gin/ginbtree.c | 3 ++- src/backend/access/gin/ginfast.c | 2 +- src/backend/access/hash/hash.c | 10 ++++++--- src/backend/access/hash/hashovfl.c | 9 +++++--- src/backend/access/transam/xloginsert.c | 3 ++- src/backend/storage/buffer/bufmgr.c | 30 +++++++++++++++++++++++++ src/include/access/xloginsert.h | 2 ++ src/include/storage/bufmgr.h | 1 + 8 files changed, 51 insertions(+), 9 deletions(-) diff --git a/src/backend/access/gin/ginbtree.c b/src/backend/access/gin/ginbtree.c index 06e97a7fbb..c98176af77 100644 --- a/src/backend/access/gin/ginbtree.c +++ b/src/backend/access/gin/ginbtree.c @@ -389,7 +389,8 @@ ginPlaceToPage(GinBtree btree, GinBtreeStack *stack, if (RelationNeedsWAL(btree->index) && !btree->isBuild) { XLogBeginInsert(); - XLogRegisterBuffer(0, stack->buffer, REGBUF_STANDARD); + XLogRegisterBuffer(0, stack->buffer, + REGBUF_STANDARD | REGBUF_CLEAN_OK); if (BufferIsValid(childbuf)) XLogRegisterBuffer(1, childbuf, REGBUF_STANDARD); } diff --git a/src/backend/access/gin/ginfast.c b/src/backend/access/gin/ginfast.c index eb6c554831..e46e11df07 100644 --- a/src/backend/access/gin/ginfast.c +++ b/src/backend/access/gin/ginfast.c @@ -399,7 +399,7 @@ ginHeapTupleFastInsert(GinState *ginstate, GinTupleCollector *collector) Assert((ptr - collectordata) <= collector->sumsize); if (needWal) { - XLogRegisterBuffer(1, buffer, REGBUF_STANDARD); + XLogRegisterBuffer(1, buffer, REGBUF_STANDARD | REGBUF_CLEAN_OK); XLogRegisterBufData(1, collectordata, collector->sumsize); } diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c index fc5d97f606..7a62100c89 100644 --- a/src/backend/access/hash/hash.c +++ b/src/backend/access/hash/hash.c @@ -824,11 +824,15 @@ hashbucketcleanup(Relation rel, Bucket cur_bucket, Buffer bucket_buf, XLogRegisterData((char *) &xlrec, SizeOfHashDelete); /* - * bucket buffer needs to be registered to ensure that we can - * acquire a cleanup lock on it during replay. + * 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 (!xlrec.is_primary_bucket_page) - XLogRegisterBuffer(0, bucket_buf, REGBUF_STANDARD | REGBUF_NO_IMAGE); + { + uint8 flags = REGBUF_STANDARD | REGBUF_NO_IMAGE | REGBUF_CLEAN_OK; + XLogRegisterBuffer(0, bucket_buf, flags); + } XLogRegisterBuffer(1, buf, REGBUF_STANDARD); XLogRegisterBufData(1, (char *) deletable, diff --git a/src/backend/access/hash/hashovfl.c b/src/backend/access/hash/hashovfl.c index 39bb2cb9f6..57688e26b5 100644 --- a/src/backend/access/hash/hashovfl.c +++ b/src/backend/access/hash/hashovfl.c @@ -658,11 +658,14 @@ _hash_freeovflpage(Relation rel, Buffer bucketbuf, Buffer ovflbuf, XLogRegisterData((char *) &xlrec, SizeOfHashSqueezePage); /* - * bucket buffer needs to be registered to ensure that we can acquire - * a cleanup lock on it during replay. + * 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 (!xlrec.is_prim_bucket_same_wrt) - XLogRegisterBuffer(0, bucketbuf, REGBUF_STANDARD | REGBUF_NO_IMAGE); + { + uint8 flags = REGBUF_STANDARD | REGBUF_NO_IMAGE | REGBUF_CLEAN_OK; + XLogRegisterBuffer(0, bucketbuf, flags); + } XLogRegisterBuffer(1, wbuf, REGBUF_STANDARD); if (xlrec.ntups > 0) diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c index 588626424e..80df735897 100644 --- a/src/backend/access/transam/xloginsert.c +++ b/src/backend/access/transam/xloginsert.c @@ -246,6 +246,7 @@ XLogRegisterBuffer(uint8 block_id, Buffer buffer, uint8 flags) /* NO_IMAGE doesn't make sense with FORCE_IMAGE */ Assert(!((flags & REGBUF_FORCE_IMAGE) && (flags & (REGBUF_NO_IMAGE)))); + Assert((flags & REGBUF_CLEAN_OK) || BufferIsDirty(buffer)); Assert(begininsert_called); if (block_id >= max_registered_block_id) @@ -1313,8 +1314,8 @@ log_newpage_range(Relation rel, ForkNumber forknum, START_CRIT_SECTION(); for (i = 0; i < nbufs; i++) { - XLogRegisterBuffer(i, bufpack[i], flags); MarkBufferDirty(bufpack[i]); + XLogRegisterBuffer(i, bufpack[i], flags); } recptr = XLogInsert(RM_XLOG_ID, XLOG_FPI); diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 8b96759b84..23b80322a7 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -2098,6 +2098,36 @@ ExtendBufferedRelShared(BufferManagerRelation bmr, return first_block; } +/* + * BufferIsDirty + * + * Checks if buffer is already dirty. + * + * Buffer must be pinned and exclusive-locked. (If caller does not hold + * exclusive lock, then the result may be stale before it's returned.) + */ +bool +BufferIsDirty(Buffer buffer) +{ + BufferDesc *bufHdr; + + if (BufferIsLocal(buffer)) + { + int bufid = -buffer - 1; + bufHdr = GetLocalBufferDescriptor(bufid); + } + else + { + bufHdr = GetBufferDescriptor(buffer - 1); + } + + Assert(BufferIsPinned(buffer)); + Assert(LWLockHeldByMeInMode(BufferDescriptorGetContentLock(bufHdr), + LW_EXCLUSIVE)); + + return pg_atomic_read_u32(&bufHdr->state) & BM_DIRTY; +} + /* * MarkBufferDirty * diff --git a/src/include/access/xloginsert.h b/src/include/access/xloginsert.h index 31785dc578..3469890cf1 100644 --- a/src/include/access/xloginsert.h +++ b/src/include/access/xloginsert.h @@ -37,6 +37,8 @@ * will be skipped) */ #define REGBUF_KEEP_DATA 0x10 /* include data even if a full-page image * is taken */ +#define REGBUF_CLEAN_OK 0x20 /* intentionally register buffer that may + * be clean */ /* prototypes for public functions in xloginsert.c: */ extern void XLogBeginInsert(void); diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h index d89021f918..8103113f09 100644 --- a/src/include/storage/bufmgr.h +++ b/src/include/storage/bufmgr.h @@ -179,6 +179,7 @@ extern Buffer ReadBufferWithoutRelcache(RelFileLocator rlocator, bool permanent); extern void ReleaseBuffer(Buffer buffer); extern void UnlockReleaseBuffer(Buffer buffer); +extern bool BufferIsDirty(Buffer buffer); extern void MarkBufferDirty(Buffer buffer); extern void IncrBufferRefCount(Buffer buffer); extern void CheckBufferIsPinnedOnce(Buffer buffer); -- 2.34.1