Re: Problem while setting the fpw with SIGHUP

Thank you, Amit, Michael.

At Sun, 29 Jul 2018 08:19:11 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20180728231911.GB1471@paquier.xyz>
> On Sat, Jul 28, 2018 at 07:10:24PM +0530, Amit Kapila wrote:
> > I have just responded to your first patch (0001).  Can you once again
> > summarize what the 0002 exactly accomplishes?  I think one of the
> > goals is to fix the original problem reported in this thread and other
> > is you have found the concurrency issue.  Is it possible to have
> > separate patches for those or you think they are interrelated and
> > needs to be fixed together?
> 
> That would be nice.  The last time I read this thread I have been rather
> confused about what was being discussed, what were the set of problems,
> and what was being fixed.  Speaking of which, this is one of the bugfix
> patches I wanted to look at once I have untanggled the autovacuum one
> for temporary relations and the DOS issues with lock lookups.

It's a long time ago.. Let me have a bit of time to blow dust off..

https://www.postgresql.org/message-id/20180420.173354.43303926.horiguchi.kyotaro@lab.ntt.co.jp

...done..i working..

The original problem here was UpdateFullPageWrites() can call
RecoveryInProgress(), which does palloc in a critical
section. This happens when standy is commanded to reload with
change of full_pages_writes during recovery.

While exploring it, I found that update of fullPageWrite flag is
updated concurrently against its design. A race condition happens
in the following steps in my diagnosis.

1. While the startup process is running recovery, we didn't
 consider that checkpointer may be running concurrently, but it
 happens for standby.

2. Checkpointer considers itself (or designed) as the *only*
 updator of shared config including fillPageWrites. In reality
 the startup is another concurent updator on standby. Addition to
 that, checkpointer assumes that it is started under WAL-emitting
 state, that is, InitXLogInsert() has been called elsewhere, but
 it is not the case on standby.

 Note that checkpointer mustn't update FPW flag before recovery
 ends because startup will overrides the flag.

3. As the result, when standby gets full_page_writes changed and
 SIGHUP during recovery, checkpointer tries to update FPW flag
 and calls RecoveryInProgress() on the way. bang!


With the 0002-step1.patch, checkpointer really becomes the only
updator of the FPW flag after recovery ends. StartupXLOG()
updates it just once before checkpointer starts to update it.

Under the normal(slow?) promotion mode, checkpointer is woken up
explicitly to make the config setting effective as soon as
possible. (This might be unnecessary.)

In checkpointer, RecoveryInProgress() is called preior to
UpdateFPW() to disable update FPW during recovery, so the crash
that is the issue here is fixed.

FYI I think that 0002-tesp2.patch is rejected.

Please find the attacehd revised patch. It is rebased and
provided with a commit message.


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From c839e039d4ebb06c0dd345f7992a460a27827805 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Wed, 1 Aug 2018 15:29:01 +0900
Subject: [PATCH] Fix FPW flags updates during recovery.

Each of checkpointer and startup processes thinks it is the only
updator of fullPageWrites flag ignorantly of each other. This causes
race condition and leads checkpionter to a crash in a certain case.

This patch makes chackpionter the only updator and organize fix the
sequence around recovery end in order to fullPageWrite flag is
correctly updated.
---
 src/backend/access/transam/xlog.c     | 19 ++++++++-----
 src/backend/postmaster/checkpointer.c | 50 ++++++++++++++++++++++++-----------
 src/include/postmaster/bgwriter.h     |  1 +
 3 files changed, 48 insertions(+), 22 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 493f1db7b9..f315d11745 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7692,14 +7692,15 @@ StartupXLOG(void)
     XLogCtl->LogwrtRqst.Flush = EndOfLog;
 
     /*
-     * Update full_page_writes in shared memory and write an XLOG_FPW_CHANGE
-     * record before resource manager writes cleanup WAL records or checkpoint
-     * record is written.
+     * Update full_page_writes in shared memory with the lastest value before
+     * resource manager writes cleanup WAL records or checkpoint record is
+     * written. We don't need to write XLOG_FPW_CHANGE since this just
+     * reflects the status at the last redo'ed record. No lock is required
+     * since startup is the only updator of the flag at this
+     * point. Checkpointer will take over after SharedRecoveryInProgress is
+     * turned to false.
      */
     Insert->fullPageWrites = lastFullPageWrites;
-    LocalSetXLogInsertAllowed();
-    UpdateFullPageWrites();
-    LocalXLogInsertAllowed = -1;
 
     if (InRecovery)
     {
@@ -7941,10 +7942,14 @@ StartupXLOG(void)
      * If this was a fast promotion, request an (online) checkpoint now. This
      * 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.
+     * than is appropriate now that we're not in standby mode anymore.  If
+     * checkpoint is not needed, tell checkpointer to update 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 de1b22d045..892f597cde 100644
--- a/src/backend/postmaster/checkpointer.c
+++ b/src/backend/postmaster/checkpointer.c
@@ -358,20 +358,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 variables are omitted during
+          * recovery. UpdateSharedMemoryConfig() is needed regardless of SIGHUP
+          * in order to keep them in sync without waiting the next reload.
+         */
+        UpdateSharedMemoryConfig();
+
         if (checkpoint_requested)
         {
             checkpoint_requested = false;
@@ -1080,6 +1085,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
@@ -1351,9 +1369,11 @@ 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. We don't do this until
+     * recovery ends.
      */
-    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