I committed and backported 0001 (the GenericXLogFinish() fix but not
the Assert).
Strangely I didn't see the -committers email come through yet. If
anyone notices anything wrong, please let me know before the final v11
release.
On Thu, 2023-09-28 at 12:05 -0700, Jeff Davis wrote:
> Also, I ran into some problems with GIN that might require a bit more
> refactoring in ginPlaceToPage(). Perhaps we could consider
> REGBUF_NO_CHANGE a general bypass of the Assert(BufferIsDirty()), and
> use it temporarily until we have a chance to analyze/refactor each of
> the callers that need it.
For the Assert, I have attached a new patch for v17.
I changed the name of the flag to REGBUF_CLEAN_OK, because for some of
the callsites it was not clear to me whether the page is always
unchanged or sometimes unchanged. In other words, REGBUF_CLEAN_OK is a
way to bypass the Assert for callsites where we either know that we
actually want to register an unchanged page, or for callsites where we
don't know if the page is changed or not.
If we commit this, ideally someone would look into the places where
REGBUF_CLEAN_OK is used and make sure that it's not a bug, and perhaps
refactor so that it would benefit from the Assert. But refactoring
those places is outside of the scope of this patch.
It sounds like we have no intention to change the hash index code, so
are we OK if this flag just lasts forever? Do you still think it offers
a useful check?
Regards,
Jeff Davis
From 859c8f4284de1030ab070630fa06af3c171264d9 Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
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-3adc31e1a...@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
