On 2020/04/22 6:57, Alvaro Herrera wrote:
On 2020-Apr-20, Fujii Masao wrote:

+       /*
+        * In 9.1 and 9.2 the postmaster unlinked the promote file inside the
+        * signal handler. It now leaves the file in place and lets the
+        * Startup process do the unlink.
+        */
+       if (IsPromoteSignaled() && stat(PROMOTE_SIGNAL_FILE, &stat_buf) == 0)
        {
-               /*
-                * In 9.1 and 9.2 the postmaster unlinked the promote file 
inside the
-                * signal handler. It now leaves the file in place and lets the
-                * Startup process do the unlink. This allows Startup to know 
whether
-                * it should create a full checkpoint before starting up 
(fallback
-                * mode). Fast promotion takes precedence.
-                */

It seems pointless to leave a very old comment that documents what the
code no longer does.  I thikn it would be better to reword it indicating
what the code does do, ie. something like "Leave the signal file in
place; it will be removed by the startup process when ..."

Agreed. And, while reading the related code, I thought that it's more proper
to place this comment in CheckPromoteSignal() rather than
CheckForStandbyTrigger(). Because CheckPromoteSignal() actually does
what the comment says, i.e., leaves the promote signal file in place and
lets the startup process do the unlink.

Attached is the updated version of the patch.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/src/backend/access/transam/xlog.c 
b/src/backend/access/transam/xlog.c
index 11e32733c4..43b6d8da69 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -298,9 +298,6 @@ bool                wal_receiver_create_temp_slot = false;
 /* are we currently in standby mode? */
 bool           StandbyMode = false;
 
-/* whether request for fast promotion has been made yet */
-static bool fast_promote = false;
-
 /*
  * if recoveryStopsBefore/After returns true, it saves information of the stop
  * point here
@@ -6311,7 +6308,7 @@ StartupXLOG(void)
        DBState         dbstate_at_startup;
        XLogReaderState *xlogreader;
        XLogPageReadPrivate private;
-       bool            fast_promoted = false;
+       bool            promoted = false;
        struct stat st;
 
        /*
@@ -7698,14 +7695,14 @@ StartupXLOG(void)
                 * the rule that TLI only changes in shutdown checkpoints, which
                 * allows some extra error checking in xlog_redo.
                 *
-                * In fast promotion, only create a lightweight end-of-recovery 
record
+                * In promotion, only create a lightweight end-of-recovery 
record
                 * instead of a full checkpoint. A checkpoint is requested 
later,
                 * after we're fully out of recovery mode and already accepting
                 * queries.
                 */
                if (bgwriterLaunched)
                {
-                       if (fast_promote)
+                       if (LocalPromoteIsTriggered)
                        {
                                checkPointLoc = ControlFile->checkPoint;
 
@@ -7716,7 +7713,7 @@ StartupXLOG(void)
                                record = ReadCheckpointRecord(xlogreader, 
checkPointLoc, 1, false);
                                if (record != NULL)
                                {
-                                       fast_promoted = true;
+                                       promoted = true;
 
                                        /*
                                         * Insert a special WAL record to mark 
the end of
@@ -7733,7 +7730,7 @@ StartupXLOG(void)
                                }
                        }
 
-                       if (!fast_promoted)
+                       if (!promoted)
                                RequestCheckpoint(CHECKPOINT_END_OF_RECOVERY |
                                                                  
CHECKPOINT_IMMEDIATE |
                                                                  
CHECKPOINT_WAIT);
@@ -7924,12 +7921,12 @@ StartupXLOG(void)
        WalSndWakeup();
 
        /*
-        * If this was a fast promotion, request an (online) checkpoint now. 
This
+        * If this was a 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.
         */
-       if (fast_promoted)
+       if (promoted)
                RequestCheckpoint(CHECKPOINT_FORCE);
 }
 
@@ -12532,29 +12529,10 @@ CheckForStandbyTrigger(void)
        if (LocalPromoteIsTriggered)
                return true;
 
-       if (IsPromoteSignaled())
+       if (IsPromoteSignaled() && stat(PROMOTE_SIGNAL_FILE, &stat_buf) == 0)
        {
-               /*
-                * In 9.1 and 9.2 the postmaster unlinked the promote file 
inside the
-                * signal handler. It now leaves the file in place and lets the
-                * Startup process do the unlink. This allows Startup to know 
whether
-                * it should create a full checkpoint before starting up 
(fallback
-                * mode). Fast promotion takes precedence.
-                */
-               if (stat(PROMOTE_SIGNAL_FILE, &stat_buf) == 0)
-               {
-                       unlink(PROMOTE_SIGNAL_FILE);
-                       unlink(FALLBACK_PROMOTE_SIGNAL_FILE);
-                       fast_promote = true;
-               }
-               else if (stat(FALLBACK_PROMOTE_SIGNAL_FILE, &stat_buf) == 0)
-               {
-                       unlink(FALLBACK_PROMOTE_SIGNAL_FILE);
-                       fast_promote = false;
-               }
-
                ereport(LOG, (errmsg("received promote request")));
-
+               unlink(PROMOTE_SIGNAL_FILE);
                ResetPromoteSignaled();
                SetPromoteIsTriggered();
                return true;
@@ -12569,7 +12547,6 @@ CheckForStandbyTrigger(void)
                                (errmsg("promote trigger file found: %s", 
PromoteTriggerFile)));
                unlink(PromoteTriggerFile);
                SetPromoteIsTriggered();
-               fast_promote = true;
                return true;
        }
        else if (errno != ENOENT)
@@ -12588,7 +12565,6 @@ void
 RemovePromoteSignalFiles(void)
 {
        unlink(PROMOTE_SIGNAL_FILE);
-       unlink(FALLBACK_PROMOTE_SIGNAL_FILE);
 }
 
 /*
@@ -12600,8 +12576,11 @@ CheckPromoteSignal(void)
 {
        struct stat stat_buf;
 
-       if (stat(PROMOTE_SIGNAL_FILE, &stat_buf) == 0 ||
-               stat(FALLBACK_PROMOTE_SIGNAL_FILE, &stat_buf) == 0)
+       /*
+        * Leave the promote signal file in place and let the Startup
+        * process do the unlink.
+        */
+       if (stat(PROMOTE_SIGNAL_FILE, &stat_buf) == 0)
                return true;
 
        return false;
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index f41084d2db..b0f771baa7 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -1196,11 +1196,6 @@ do_promote(void)
                exit(1);
        }
 
-       /*
-        * For 9.3 onwards, "fast" promotion is performed. Promotion with a full
-        * checkpoint is still possible by writing a file called
-        * "fallback_promote" instead of "promote"
-        */
        snprintf(promote_file, MAXPGPATH, "%s/promote", pg_data);
 
        if ((prmfile = fopen(promote_file, "w")) == NULL)
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index f60ed2d36c..3914049438 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -382,6 +382,5 @@ extern SessionBackupState get_backup_status(void);
 
 /* files to signal promotion to primary */
 #define PROMOTE_SIGNAL_FILE            "promote"
-#define FALLBACK_PROMOTE_SIGNAL_FILE  "fallback_promote"
 
 #endif                                                 /* XLOG_H */

Reply via email to