On Wed, 2023-09-27 at 00:14 +0300, Heikki Linnakangas wrote:
> Looks correct. You now loop through all the block IDs three times,
> however. I wonder if that is measurably slower, but even if it's not,
> was there a reason you wanted to move the XLogRegisterBuffer() calls
> to
> a separate loop?
I did so to correspond more closely to what's outlined in the README
and in other places in the code, where marking the buffers dirty
happens before XLogBeginInsert(). It didn't occur to me that one extra
loop would matter, but I can combine them again.
It would be a bit more concise to do the XLogBeginInsert() first (like
before) and then register the buffers in the same loop that does the
writes and marks the buffers dirty. Updated patch attached.
>
> Hmm, I'm sure there are exceptions but log_newpage_range() actually
> seems to be doing the right thing; it calls MarkBufferDirty() before
> XLogInsert(). It only calls it after XLogRegisterBuffer() though, and
> I
> concur that XLogRegisterBuffer() would be the logical place for the
> assertion. We could tighten this up, require that you call
> MarkBufferDirty() before XLogRegisterBuffer(), and fix all the
> callers.
That site is pretty trivial to fix, but there are also a couple places
in hash.c and hashovfl.c that are registering a clean page and it's not
clear to me exactly what's going on.
Regards,
Jeff Davis
From 9548bb49865db3a04bbdda4c63df611142e22964 Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Tue, 26 Sep 2023 12:10:11 -0700
Subject: [PATCH v2] Fix GenericXLogFinish().
Mark the buffers dirty before writing WAL.
Discussion: https://postgr.es/m/25104133-7df8-cae3-b9a2-1c0aaa1c0...@iki.fi
---
src/backend/access/transam/generic_xlog.c | 53 ++++++++++-------------
1 file changed, 23 insertions(+), 30 deletions(-)
diff --git a/src/backend/access/transam/generic_xlog.c b/src/backend/access/transam/generic_xlog.c
index 6c68191ca6..2d5ff4aa7c 100644
--- a/src/backend/access/transam/generic_xlog.c
+++ b/src/backend/access/transam/generic_xlog.c
@@ -347,6 +347,10 @@ GenericXLogFinish(GenericXLogState *state)
START_CRIT_SECTION();
+ /*
+ * Compute deltas if necessary, write changes to buffers, mark
+ * buffers dirty, and register changes.
+ */
for (i = 0; i < MAX_GENERIC_XLOG_PAGES; i++)
{
PageData *pageData = &state->pages[i];
@@ -359,41 +363,31 @@ GenericXLogFinish(GenericXLogState *state)
page = BufferGetPage(pageData->buffer);
pageHeader = (PageHeader) pageData->image;
+ /* delta not needed for a full page image */
+ if (!(pageData->flags & GENERIC_XLOG_FULL_IMAGE))
+ computeDelta(pageData, page, (Page) pageData->image);
+
+ /*
+ * Apply the image, being careful to zero the "hole" between
+ * pd_lower and pd_upper in order to avoid divergence between
+ * actual page state and what replay would produce.
+ */
+ memcpy(page, pageData->image, pageHeader->pd_lower);
+ memset(page + pageHeader->pd_lower, 0,
+ pageHeader->pd_upper - pageHeader->pd_lower);
+ memcpy(page + pageHeader->pd_upper,
+ pageData->image + pageHeader->pd_upper,
+ BLCKSZ - pageHeader->pd_upper);
+
+ MarkBufferDirty(pageData->buffer);
+
if (pageData->flags & GENERIC_XLOG_FULL_IMAGE)
{
- /*
- * A full-page image does not require us to supply any xlog
- * data. Just apply the image, being careful to zero the
- * "hole" between pd_lower and pd_upper in order to avoid
- * divergence between actual page state and what replay would
- * produce.
- */
- memcpy(page, pageData->image, pageHeader->pd_lower);
- memset(page + pageHeader->pd_lower, 0,
- pageHeader->pd_upper - pageHeader->pd_lower);
- memcpy(page + pageHeader->pd_upper,
- pageData->image + pageHeader->pd_upper,
- BLCKSZ - pageHeader->pd_upper);
-
XLogRegisterBuffer(i, pageData->buffer,
REGBUF_FORCE_IMAGE | REGBUF_STANDARD);
}
else
{
- /*
- * In normal mode, calculate delta and write it as xlog data
- * associated with this page.
- */
- computeDelta(pageData, page, (Page) pageData->image);
-
- /* Apply the image, with zeroed "hole" as above */
- memcpy(page, pageData->image, pageHeader->pd_lower);
- memset(page + pageHeader->pd_lower, 0,
- pageHeader->pd_upper - pageHeader->pd_lower);
- memcpy(page + pageHeader->pd_upper,
- pageData->image + pageHeader->pd_upper,
- BLCKSZ - pageHeader->pd_upper);
-
XLogRegisterBuffer(i, pageData->buffer, REGBUF_STANDARD);
XLogRegisterBufData(i, pageData->delta, pageData->deltaLen);
}
@@ -402,7 +396,7 @@ GenericXLogFinish(GenericXLogState *state)
/* Insert xlog record */
lsn = XLogInsert(RM_GENERIC_ID, 0);
- /* Set LSN and mark buffers dirty */
+ /* Set LSN */
for (i = 0; i < MAX_GENERIC_XLOG_PAGES; i++)
{
PageData *pageData = &state->pages[i];
@@ -410,7 +404,6 @@ GenericXLogFinish(GenericXLogState *state)
if (BufferIsInvalid(pageData->buffer))
continue;
PageSetLSN(BufferGetPage(pageData->buffer), lsn);
- MarkBufferDirty(pageData->buffer);
}
END_CRIT_SECTION();
}
--
2.34.1
