Re: Problem while setting the fpw with SIGHUP - Mailing list pgsql-hackers

By the way, I think I found a bug of FPW.

The following steps yields INSERT record that doesn't have a FPI
after a checkpoint.

(Start server with full_page_writes = off)
CREATE TABLE t (a int);
CHECKPOINT;
INSERT INTO t VALUES (1);
ALTER SYSTEM SET full_page_writes TO on;
SELECT pg_reload_conf();
CHECKPOINT;
INSERT INTO t VALUES (1);

The last insert is expected to write a record with FPI but it
doesn't actually. No FPI will be written for the page after that.

It seems that the reason is that XLogInsertRecord is forgetting
to check doPageWrites' update.

In the failure case, fpw_lsn has been set by XLogRecordAssemble
but doPageWrite is false at the time and it considers that no FPI
is required then it sets fpw_lsn to InvalidXLogRecPtr.

After that, XLogInsertRecord receives the record but it thinks
that doPageWrites is true since it looks the shared value.

> if (fpw_lsn != InvalidXLogRecPtr && fpw_lsn <= RedoRecPtr && doPageWrites)

So this line thinks that "no FPI is omitted in this record" but
actually the record is just forgotten to attach them.

The attached patch lets XLogInsertRecord check if doPageWrites
has been turned on after the last call and cause recomputation in
the case.

> * If there are any registered buffers, and a full-page image was not taken
> * of all of them, *fpw_lsn is set to the lowest LSN among such pages. This
> * signals that the assembled record is only good for insertion on the
> * assumption that the RedoRecPtr and doPageWrites values were up-to-date.

And the patch fixes one comment typo of XLogInsertRecord.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 40ce98bba0496b1eb0a982134eae9cec01d532a8 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Fri, 20 Apr 2018 15:01:26 +0900
Subject: [PATCH] Correctly attach FPI to the first record after a checkpoint

XLogInsert fails to attach a required FPIs to the first record after a
checkpoint if no other record has been written after full_page_writes
was turned on. This makes incosistency between fpw flag of the
checkpoint record and the following record's FPW status. This patch
makes XLogInsertRecord to cause a recomputation when the given record
is generated during FPW is off but found that the flag has been turned
on.
---
 src/backend/access/transam/xlog.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 08dc9ba031..27753f7321 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -937,7 +937,7 @@ static void WALInsertLockUpdateInsertingAt(XLogRecPtr insertingAt);
  *
  * If 'fpw_lsn' is valid, it is the oldest LSN among the pages that this
  * WAL record applies to, that were not included in the record as full page
- * images.  If fpw_lsn >= RedoRecPtr, the function does not perform the
+ * images.  If fpw_lsn <= RedoRecPtr, the function does not perform the
  * insertion and returns InvalidXLogRecPtr.  The caller can then recalculate
  * which pages need a full-page image, and retry.  If fpw_lsn is invalid, the
  * record is always inserted.
@@ -970,6 +970,7 @@ XLogInsertRecord(XLogRecData *rdata,
                                info == XLOG_SWITCH);
     XLogRecPtr    StartPos;
     XLogRecPtr    EndPos;
+    bool        prevDoPageWrites = doPageWrites;
 
     /* we assume that all of the record header is in the first chunk */
     Assert(rdata->len >= SizeOfXLogRecord);
@@ -1022,7 +1023,8 @@ XLogInsertRecord(XLogRecData *rdata,
      * This can only happen just after a checkpoint, so it's better to be slow
      * in this case and fast otherwise.
      *
-     * If we aren't doing full-page writes then RedoRecPtr doesn't actually
+     * If doPageWrites was just turned on, we force a recomputation. Otherwise
+     * if we aren't doing full-page writes then RedoRecPtr doesn't actually
      * affect the contents of the XLOG record, so we'll update our local copy
      * but not force a recomputation.  (If doPageWrites was just turned off,
      * we could recompute the record without full pages, but we choose not to
@@ -1035,7 +1037,9 @@ XLogInsertRecord(XLogRecData *rdata,
     }
     doPageWrites = (Insert->fullPageWrites || Insert->forcePageWrites);
 
-    if (fpw_lsn != InvalidXLogRecPtr && fpw_lsn <= RedoRecPtr && doPageWrites)
+    if (doPageWrites &&
+        (!prevDoPageWrites ||
+         (fpw_lsn != InvalidXLogRecPtr && fpw_lsn <= RedoRecPtr)))
     {
         /*
          * Oops, some buffer now needs to be backed up that the caller didn't
-- 
2.16.3