On Mon, 2023-09-25 at 13:04 +0300, Heikki Linnakangas wrote:
> Yes, that's a problem.
Patch attached. I rearranged the code a bit to follow the expected
pattern of: write, mark dirty, WAL, set LSN. I think computing the
deltas could also be moved earlier, outside of the critical section,
but I'm not sure that would be useful.
Do you have a suggestion for any kind of test addition, or should we
just review carefully?
> I wish we had an assertion for that. XLogInsert() could assert that
> the
> page is already marked dirty, for example.
Unfortunately that's not always the case, e.g. log_newpage_range().
Regards,
Jeff Davis
From c54749a7939721f0a838115abce09967216cbc0f 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 v1] 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 | 66 ++++++++++++-----------
1 file changed, 34 insertions(+), 32 deletions(-)
diff --git a/src/backend/access/transam/generic_xlog.c b/src/backend/access/transam/generic_xlog.c
index 6c68191ca6..2b7e054ebe 100644
--- a/src/backend/access/transam/generic_xlog.c
+++ b/src/backend/access/transam/generic_xlog.c
@@ -343,10 +343,12 @@ GenericXLogFinish(GenericXLogState *state)
if (state->isLogged)
{
/* Logged relation: make xlog record in critical section. */
- XLogBeginInsert();
-
START_CRIT_SECTION();
+ /*
+ * Compute deltas if necessary, write changes to buffers, and mark
+ * them dirty.
+ */
for (i = 0; i < MAX_GENERIC_XLOG_PAGES; i++)
{
PageData *pageData = &state->pages[i];
@@ -359,41 +361,42 @@ 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);
+ }
+
+ XLogBeginInsert();
+
+ /* Register buffers */
+ for (i = 0; i < MAX_GENERIC_XLOG_PAGES; i++)
+ {
+ PageData *pageData = &state->pages[i];
+
+ if (BufferIsInvalid(pageData->buffer))
+ continue;
+
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 +405,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 +413,6 @@ GenericXLogFinish(GenericXLogState *state)
if (BufferIsInvalid(pageData->buffer))
continue;
PageSetLSN(BufferGetPage(pageData->buffer), lsn);
- MarkBufferDirty(pageData->buffer);
}
END_CRIT_SECTION();
}
--
2.34.1
