Re: 回复： Why is XLOG_FPI_FOR_HINT always need backups?

At Sat, 17 Jul 2021 00:14:34 +0900, Fujii Masao <masao.fujii@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.ntt@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