Re: Problem while setting the fpw with SIGHUP

At Fri, 13 Apr 2018 13:47:51 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20180413.134751.76149471.horiguchi.kyotaro@lab.ntt.co.jp>
> At Fri, 13 Apr 2018 08:31:02 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in
<CAA4eK1LVFpLf=d-7XmfwhLv7Xu53pU0bGU=wVrYWSRU4XSsyHQ@mail.gmail.com>
> > On Fri, Apr 13, 2018 at 6:59 AM, Michael Paquier <michael@paquier.xyz> wrote:
> > > On Thu, Apr 12, 2018 at 02:55:53PM -0400, Robert Haas wrote:
> > >> I think it may actually be confusing.  If you run pg_ctl reload and it
> > >> reports that the value has changed, you'll expect it to have taken
> > >> effect.  But really, it will take effect at some later time.
> > >
> > 
> > +1. I also think it is confusing and it could be difficult for end
> > users to know when the setting is effective.
> > 
> > > It is true that sometimes some people like to temporarily disable
> > > full_page_writes particularly when doing some bulk load of data to
> > > minimize the effort on WAL, and then re-enable it just after doing
> > > the inserting this data.
> > >
> > > Still does it matter when the change is effective?  By disabling
> > > full_page_writes even temporarily, you accept the fact that this
> > > instance would not be safe until the next checkpoint completes.  The
> > > instance even finishes by writing less unnecessary WAL data if the
> > > change is only effective at the next checkpoint.  Well, it is true that
> > > this increases potential torn pages problems but the user is already
> > > accepting that risk if a crash happens until the next checkpoint then it
> > > exposes itself to torn pages anyway as it chose to disable
> > > full_page_writes.
> 
> I still don't think that enabling FPW anytime is useful but
> disabling seems useful as I mentioned upthread.
> 
> The problem was checkpointer changes the flag anytime including
> recovery time. Startup process updates the same flag at the end
> of recovery but before publicated. Letting checkpointer change
> the flag only at checkpoint time is a straightforward way to
> avoid conflicts with startup process.
> 
> I reconsider a bit and came up with the thought that we could
> just skip changing shared FPW in checkpointer until recovery
> ends, then update the flag after recovery end (perhaps at
> checkpoint time in major cases). In this case, FPI is attached
> from REDO point of the first checkpoint (not restartpoint) or a
> bit earlier, then FPW can be flipped at any time.  I'll come up
> with that soon.

Please find the attached. The most significant change is that
UpdateSharedMemoryConfig skips updating of shared fullPageWrites
during recovery. The original crash is fixed since this
guarantees that XLog working area is initializeed before reaching
UpdateFullPageWrites().

Addition to that, I changed CheckpointerMain so that it tries
update of shared FPW regardless of SIGHUP and provided new
function to just wakeup checkpointer. StartupXLOG wakes up
checkpointer either checkpoint is required or not and
checkpointer makes the first update of shared FPW at the time.

After this point, everything works as the same as the current
behavior.

> > I think this means that is will be difficult for end users to predict
> > unless they track the next checkpoint which isn't too bad, but won't
> > be convenient either.
> 
> Looking checkpiont record is enough to know wheter the checkpoint
> is protected by FPW eough, but I agree that such strictness is
> not crutial.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 70cf6a0c0fb4c4e421d8895c197b1bf7fecdaa8f Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Fri, 13 Apr 2018 16:29:25 +0900
Subject: [PATCH] Inhibit update shared FPW during recovery

The shared full_pages_writes is currently updated by checkpointer even
during recovery. This leads to unexpected concurrent updates of the
flag or a crash by using uninitialized memory. The update is needless
in the first place so this patch inhibits checkpointer from updating
it during recovery. Instead, startup process wakes up checkpointer at
the end of recovery so as to the shared flag is updated just after.
---
 src/backend/access/transam/xlog.c     |  4 +++
 src/backend/postmaster/checkpointer.c | 57 ++++++++++++++++++++++-------------
 src/include/postmaster/bgwriter.h     |  1 +
 3 files changed, 41 insertions(+), 21 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 08dc9ba031..581844904d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7826,9 +7826,13 @@ StartupXLOG(void)
      * isn't required for consistency, but the last restartpoint might be far
      * back, and in case of a crash, recovering from it might take a longer
      * than is appropriate now that we're not in standby mode anymore.
+     * If no checkpoint need, request checkpointer to process shared
+     * configuration instead.
      */
     if (fast_promoted)
         RequestCheckpoint(CHECKPOINT_FORCE);
+    else
+        WakeupCheckpointer();
 }
 
 /*
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
index 4b452e7cee..332eba1c7b 100644
--- a/src/backend/postmaster/checkpointer.c
+++ b/src/backend/postmaster/checkpointer.c
@@ -332,12 +332,6 @@ CheckpointerMain(void)
      */
     PG_SETMASK(&UnBlockSig);
 
-    /*
-     * Ensure all shared memory values are set correctly for the config. Doing
-     * this here ensures no race conditions from other concurrent updaters.
-     */
-    UpdateSharedMemoryConfig();
-
     /*
      * Advertise our latch that backends can use to wake us up while we're
      * sleeping.
@@ -368,20 +362,25 @@ CheckpointerMain(void)
         {
             got_SIGHUP = false;
             ProcessConfigFile(PGC_SIGHUP);
-
-            /*
-             * Checkpointer is the last process to shut down, so we ask it to
-             * hold the keys for a range of other tasks required most of which
-             * have nothing to do with checkpointing at all.
-             *
-             * For various reasons, some config values can change dynamically
-             * so the primary copy of them is held in shared memory to make
-             * sure all backends see the same value.  We make Checkpointer
-             * responsible for updating the shared memory copy if the
-             * parameter setting changes because of SIGHUP.
-             */
-            UpdateSharedMemoryConfig();
         }
+
+        /*
+         * Checkpointer is the last process to shut down, so we ask it to hold
+         * the keys for a range of other tasks required most of which have
+         * nothing to do with checkpointing at all.
+         *
+         * For various reasons, some config values can change dynamically so
+         * the primary copy of them is held in shared memory to make sure all
+         * backends see the same value.  We make Checkpointer responsible for
+         * updating the shared memory copy if the parameter setting changes
+         * because of SIGHUP.
+         *
+         * Since some of the shared memory copy are not updated during
+          * recovery and are to be in sync as soon as recovery ends, call
+          * UpdateSharedMemoryConfig() always regardless of SIGHUP.
+         */
+        UpdateSharedMemoryConfig();
+
         if (checkpoint_requested)
         {
             checkpoint_requested = false;
@@ -1090,6 +1089,19 @@ RequestCheckpoint(int flags)
     }
 }
 
+/*
+ * WakeupCheckpointer
+ *
+ * Requests checkpointer to wake up. This causes per-loop processing
+ * regardless of the necessity of a checkpoint.
+ */
+void
+WakeupCheckpointer(void)
+{
+    if (IsUnderPostmaster && CheckpointerShmem->checkpointer_pid != 0)
+        kill(CheckpointerShmem->checkpointer_pid, SIGINT);
+}
+
 /*
  * ForwardFsyncRequest
  *        Forward a file-fsync request from a backend to the checkpointer
@@ -1361,9 +1373,12 @@ UpdateSharedMemoryConfig(void)
 
     /*
      * If full_page_writes has been changed by SIGHUP, we update it in shared
-     * memory and write an XLOG_FPW_CHANGE record.
+     * memory and write an XLOG_FPW_CHANGE record. Don't do that during
+     * recovery since we are not allowed to write WAL and startup process has
+     * the priority to update the shared flag.
      */
-    UpdateFullPageWrites();
+    if (!RecoveryInProgress())
+        UpdateFullPageWrites();
 
     elog(DEBUG2, "checkpointer updated shared memory configuration values");
 }
diff --git a/src/include/postmaster/bgwriter.h b/src/include/postmaster/bgwriter.h
index 941c6aba7d..3aac1606a0 100644
--- a/src/include/postmaster/bgwriter.h
+++ b/src/include/postmaster/bgwriter.h
@@ -29,6 +29,7 @@ extern void BackgroundWriterMain(void) pg_attribute_noreturn();
 extern void CheckpointerMain(void) pg_attribute_noreturn();
 
 extern void RequestCheckpoint(int flags);
+extern void WakeupCheckpointer(void);
 extern void CheckpointWriteDelay(int flags, double progress);
 
 extern bool ForwardFsyncRequest(RelFileNode rnode, ForkNumber forknum,
-- 
2.16.3