On 2020/06/03 3:38, Hamid Akhtar wrote:
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested
I've applied the v2 patch on the master branch. There some hunks, but the patch
got applied. So, I ran make installcheck-world and everything looks fine to me
with this patch. Though, I do have a few suggestions in general:
Thanks for the test and review!
(1) I see two functions being used (a) CheckPromoteSignal and (b) IsPromoteSignaled in the code. Should these
be combined into a single function and perhaps check for "promote_signaled" and the
"PROMOTE_SIGNAL_FILE". Not sure if doing this will break "sigusr1_handler" in
postmaster.c though.
I don't think we can do that simply. CheckPromoteSignal() can be called by
both postmaster and the startup process. OTOH, IsPromoteSignaled()
accesses the flag that can be set only in the startup process' signal handler,
i.e., it's intended to be called only by the startup process.
(2) CheckPromoteSignal is checking for "PROMOTE_SIGNAL_FILE" file. So, perhaps, rather than calling
stat on "PROMOTE_SIGNAL_FILE" in if statements, I would suggest to use CheckPromoteSignal function
instead as it does nothing but stat on "PROMOTE_SIGNAL_FILE" (after applying your patch).
Yes, that's good idea. Attached is the updated version of the patch.
I replaced that stat() with CheckPromoteSignal(). Also I replaced
unlink(PROMOTE_SIGNAL_FILE) with RemovePromoteSignalFiles().
The new status of this patch is: Waiting on Author
I will change the status back to Needs Review.
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 ca09d81b08..1df1aa9b30 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -299,9 +299,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
@@ -6319,7 +6316,7 @@ StartupXLOG(void)
DBState dbstate_at_startup;
XLogReaderState *xlogreader;
XLogPageReadPrivate private;
- bool fast_promoted = false;
+ bool promoted = false;
struct stat st;
/*
@@ -7724,14 +7721,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;
@@ -7742,7 +7739,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
@@ -7759,7 +7756,7 @@ StartupXLOG(void)
}
}
- if (!fast_promoted)
+ if (!promoted)
RequestCheckpoint(CHECKPOINT_END_OF_RECOVERY |
CHECKPOINT_IMMEDIATE |
CHECKPOINT_WAIT);
@@ -7950,12 +7947,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);
}
@@ -12577,29 +12574,10 @@ CheckForStandbyTrigger(void)
if (LocalPromoteIsTriggered)
return true;
- if (IsPromoteSignaled())
+ if (IsPromoteSignaled() && CheckPromoteSignal())
{
- /*
- * 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")));
-
+ RemovePromoteSignalFiles();
ResetPromoteSignaled();
SetPromoteIsTriggered();
return true;
@@ -12614,7 +12592,6 @@ CheckForStandbyTrigger(void)
(errmsg("promote trigger file found: %s",
PromoteTriggerFile)));
unlink(PromoteTriggerFile);
SetPromoteIsTriggered();
- fast_promote = true;
return true;
}
else if (errno != ENOENT)
@@ -12633,20 +12610,17 @@ void
RemovePromoteSignalFiles(void)
{
unlink(PROMOTE_SIGNAL_FILE);
- unlink(FALLBACK_PROMOTE_SIGNAL_FILE);
}
/*
- * Check to see if a promote request has arrived. Should be
- * called by postmaster after receiving SIGUSR1.
+ * Check to see if a promote request has arrived.
*/
bool
CheckPromoteSignal(void)
{
struct stat stat_buf;
- if (stat(PROMOTE_SIGNAL_FILE, &stat_buf) == 0 ||
- stat(FALLBACK_PROMOTE_SIGNAL_FILE, &stat_buf) == 0)
+ if (stat(PROMOTE_SIGNAL_FILE, &stat_buf) == 0)
return true;
return false;
diff --git a/src/backend/postmaster/postmaster.c
b/src/backend/postmaster/postmaster.c
index 160afe9f39..de0dbd7f08 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -5333,7 +5333,12 @@ sigusr1_handler(SIGNAL_ARGS)
pmState == PM_HOT_STANDBY || pmState == PM_WAIT_READONLY) &&
CheckPromoteSignal())
{
- /* Tell startup process to finish recovery */
+ /*
+ * Tell startup process to finish recovery.
+ *
+ * Leave the promote signal file in place and let the Startup
+ * process do the unlink.
+ */
signal_child(StartupPID, SIGUSR2);
}
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 3c03ace7ed..1cdc3ebaa3 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -1195,11 +1195,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 e917dfe92d..657bc6b6e6 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -391,6 +391,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 */
