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

At Wed, 28 Mar 2018 15:59:48 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20180328065948.GM1105@paquier.xyz>
> On Wed, Mar 28, 2018 at 03:40:59PM +0900, Kyotaro HORIGUCHI wrote:
> > The attached does that. I don't like that it uses ControlFileLock
> > to exlucde concurrent UpdateFullPageWrites and StartupXLOG but
> > WALInsertLock cannot be used since UpdateFullPageWrites may take
> > the same lock.
> 
> You visibly forgot your patch.

Mmm, someone must have eaten that. I'm sure it is attached this
time.

I don't like UpdateFullPageWrites is using ControlFileLock to
exclusion..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index cb9c2a29cb..d2bb5e16ac 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7558,9 +7558,11 @@ StartupXLOG(void)
     /*
      * 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.
+     * record is written, ignoreing the change of full_page_write GUC so far.
      */
+    WALInsertLockAcquireExclusive();
     Insert->fullPageWrites = lastFullPageWrites;
+    WALInsertLockRelease();
     LocalSetXLogInsertAllowed();
     UpdateFullPageWrites();
     LocalXLogInsertAllowed = -1;
@@ -7783,6 +7785,9 @@ StartupXLOG(void)
      * itself, we use the info_lck here to ensure that there are no race
      * conditions concerning visibility of other recent updates to shared
      * memory.
+     *
+     * ControlFileLock excludes concurrent update of shared fullPageWrites in
+     * addition to its ordinary use.
      */
     LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
     ControlFile->state = DB_IN_PRODUCTION;
@@ -7790,11 +7795,25 @@ StartupXLOG(void)
 
     SpinLockAcquire(&XLogCtl->info_lck);
     XLogCtl->SharedRecoveryInProgress = false;
+    lastFullPageWrites = Insert->fullPageWrites;
     SpinLockRelease(&XLogCtl->info_lck);
 
     UpdateControlFile();
     LWLockRelease(ControlFileLock);
 
+    /*
+     * Shared fullPageWrites at the end of recovery differs to our last known
+     * value. The change has been made by checkpointer but we haven't made
+     * corresponding XLOG_FPW_CHANGE record. We reread GUC change if any and
+     * try update shared fullPageWrites by myself. It ends with doing nothing
+     * if checkpointer already did the same thing.
+     */
+    if (lastFullPageWrites != fullPageWrites)
+    {
+        HandleStartupProcInterrupts();
+        UpdateFullPageWrites();
+    }
+
     /*
      * If there were cascading standby servers connected to us, nudge any wal
      * sender processes to notice that we've been promoted.
@@ -9525,8 +9544,10 @@ XLogReportParameters(void)
  * Update full_page_writes in shared memory, and write an
  * XLOG_FPW_CHANGE record if necessary.
  *
- * Note: this function assumes there is no other process running
- * concurrently that could update it.
+ * This function assumes called concurrently from multiple processes and
+ * called concurrently with changing of shared fullPageWrites. See
+ * StartupXLOG().
+ * 
  */
 void
 UpdateFullPageWrites(void)
@@ -9536,13 +9557,25 @@ UpdateFullPageWrites(void)
     /*
      * Do nothing if full_page_writes has not been changed.
      *
-     * It's safe to check the shared full_page_writes without the lock,
-     * because we assume that there is no concurrently running process which
-     * can update it.
+     * We acquire ControlFileLock to exclude other concurrent call and change
+     * of shared fullPageWrites.
      */
+    LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
+    WALInsertLockAcquireExclusive();
     if (fullPageWrites == Insert->fullPageWrites)
+    {
+        WALInsertLockRelease();
+        LWLockRelease(ControlFileLock);
         return;
+    }
+    WALInsertLockRelease();
 
+    /*
+     * Need to set up XLogInsert working area before entering critical section
+     * if we are not in recovery mode.
+     */
+    (void) RecoveryInProgress();
+        
     START_CRIT_SECTION();
 
     /*
@@ -9578,6 +9611,8 @@ UpdateFullPageWrites(void)
         WALInsertLockRelease();
     }
     END_CRIT_SECTION();
+
+    LWLockRelease(ControlFileLock);
 }
 
 /*