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