At Sat, 17 Jul 2021 00:14:34 +0900, Fujii Masao <masao.fu...@oss.nttdata.com>
wrote in
> Thanks for updating the patch! It basically looks good to me.
>
> * Full-page image (FPI) records contain nothing else but a
> backup
> * block (or multiple backup blocks). Every block reference must
> * include a full-page image - otherwise there would be no
> point in
> * this record.
>
> The above comment also needs to be updated?
In short, no. In contrast to the third paragraph, the first paragraph
should be thought that it is describing XLOG_FPI. However, actually
it is not super obvious so it's better to make it clearer. Addition to
that, it seems to me (yes, to *me*) somewhat confused between "block
reference", "backup block" and "full-page image". So I'd like to
adjust the paragraph as the following.
> * XLOG_FPI records contain nothing else but one or more block
> * references. Every block reference must include a full-page image
> * regardless of the full_page_writes setting - otherwise there would
> * be no point in this record.
FYI (or, for the record), the first paragraph got to the current shape
by the commit 9155580fd5, where XLOG_FPI was modified to be able to
hold more than one block references.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From cd54ae028878fac0a97baebf71e39cb9b518e885 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota....@gmail.com>
Date: Wed, 7 Jul 2021 15:34:41 +0900
Subject: [PATCH v3] Make FPI_FOR_HINT follow standard FPI emitting policy
Commit 2c03216d831160bedd72d45f712601b6f7d03f1c changed
XLogSaveBufferForHint to enforce FPI but the caller didn't intend to
do so. Restore the intended behavior that FPI_FOR_HINT follows
full_page_writes.
---
src/backend/access/transam/xlog.c | 30 +++++++++++++++++--------
src/backend/access/transam/xloginsert.c | 5 +----
2 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 2ee9515139..ff10fef0d3 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -10148,7 +10148,10 @@ xlog_redo(XLogReaderState *record)
uint8 info = XLogRecGetInfo(record) & ~XLR_INFO_MASK;
XLogRecPtr lsn = record->EndRecPtr;
- /* in XLOG rmgr, backup blocks are only used by XLOG_FPI records */
+ /*
+ * in XLOG rmgr, backup blocks are only used by XLOG_FPI and
+ * XLOG_FPI_FOR_HINT records.
+ */
Assert(info == XLOG_FPI || info == XLOG_FPI_FOR_HINT ||
!XLogRecHasAnyBlockRefs(record));
@@ -10356,25 +10359,34 @@ xlog_redo(XLogReaderState *record)
else if (info == XLOG_FPI || info == XLOG_FPI_FOR_HINT)
{
/*
- * Full-page image (FPI) records contain nothing else but a backup
- * block (or multiple backup blocks). Every block reference must
- * include a full-page image - otherwise there would be no point in
- * this record.
+ * XLOG_FPI records contain nothing else but one or more block
+ * references. Every block reference must include a full-page image
+ * regardless of the full_page_writes setting - otherwise there would
+ * be no point in this record.
*
* No recovery conflicts are generated by these generic records - if a
* resource manager needs to generate conflicts, it has to define a
* separate WAL record type and redo routine.
*
* XLOG_FPI_FOR_HINT records are generated when a page needs to be
- * WAL- logged because of a hint bit update. They are only generated
- * when checksums are enabled. There is no difference in handling
- * XLOG_FPI and XLOG_FPI_FOR_HINT records, they use a different info
- * code just to distinguish them for statistics purposes.
+ * WAL-logged because of a hint bit update. They are only generated
+ * when checksums and/or wal_log_hints are enabled. The only difference
+ * in handling XLOG_FPI and XLOG_FPI_FOR_HINT records is that the
+ * latter is allowed to be missing actual block image. In that case the
+ * record is effectively a NOP record and should not even try to read
+ * the page from disk.
*/
for (uint8 block_id = 0; block_id <= record->max_block_id; block_id++)
{
Buffer buffer;
+ if (!XLogRecHasBlockImage(record, block_id))
+ {
+ if (info == XLOG_FPI)
+ elog(ERROR, "missing full page image in XLOG_FPI record");
+ continue;
+ }
+
if (XLogReadBufferForRedo(record, block_id, &buffer) != BLK_RESTORED)
elog(ERROR, "unexpected XLogReadBufferForRedo result when restoring backup block");
UnlockReleaseBuffer(buffer);
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index 3d2c9c3e8c..e596a0470a 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -287,8 +287,6 @@ XLogRegisterBlock(uint8 block_id, RelFileNode *rnode, ForkNumber forknum,
{
registered_buffer *regbuf;
- /* This is currently only used to WAL-log a full-page image of a page */
- Assert(flags & REGBUF_FORCE_IMAGE);
Assert(begininsert_called);
if (block_id >= max_registered_block_id)
@@ -995,7 +993,7 @@ XLogSaveBufferForHint(Buffer buffer, bool buffer_std)
if (lsn <= RedoRecPtr)
{
- int flags;
+ int flags = 0;
PGAlignedBlock copied_buffer;
char *origdata = (char *) BufferGetBlock(buffer);
RelFileNode rnode;
@@ -1022,7 +1020,6 @@ XLogSaveBufferForHint(Buffer buffer, bool buffer_std)
XLogBeginInsert();
- flags = REGBUF_FORCE_IMAGE;
if (buffer_std)
flags |= REGBUF_STANDARD;
--
2.27.0
