On 2020/10/05 19:45, Bharath Rupireddy wrote:
Hi,
Autoprewarm module is using it's own SIGHUP(apw_sigterm_handler, got_sigterm)
and SIGTERM(apw_sighup_handler, got_sighup) handlers which are similar to
standard signal handlers(except for a difference [1]). Isn't it good to remove
them and use standard SignalHandlerForConfigReload and
SignalHandlerForShutdownRequest?
Attaching the patch for the above changes.
Looks like the commit[2] replaced custom handlers with standard handlers.
Thoughts?
+1
The patch looks good to me.
ISTM that we can also replace StartupProcSigHupHandler() in startup.c
with SignalHandlerForConfigReload() by making the startup process use
the general shared latch instead of its own one. POC patch attached.
Thought?
Probably we can also replace sigHupHandler() in syslogger.c with
SignalHandlerForConfigReload()? This would be separate patch, though.
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 8f11b1b9de..8b4434962f 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -682,7 +682,7 @@ typedef struct XLogCtlData
* WAL replay, if it is waiting for WAL to arrive or failover trigger
file
* to appear.
*/
- Latch recoveryWakeupLatch;
+ Latch *recoveryWakeupLatch;
/*
* During recovery, we keep a copy of the latest checkpoint record here.
@@ -5185,7 +5185,6 @@ XLOGShmemInit(void)
SpinLockInit(&XLogCtl->Insert.insertpos_lck);
SpinLockInit(&XLogCtl->info_lck);
SpinLockInit(&XLogCtl->ulsn_lck);
- InitSharedLatch(&XLogCtl->recoveryWakeupLatch);
}
/*
@@ -6122,7 +6121,7 @@ recoveryApplyDelay(XLogReaderState *record)
while (true)
{
- ResetLatch(&XLogCtl->recoveryWakeupLatch);
+ ResetLatch(MyLatch);
/* might change the trigger file's location */
HandleStartupProcInterrupts();
@@ -6146,7 +6145,7 @@ recoveryApplyDelay(XLogReaderState *record)
elog(DEBUG2, "recovery apply delay %ld seconds, %d
milliseconds",
secs, microsecs / 1000);
- (void) WaitLatch(&XLogCtl->recoveryWakeupLatch,
+ (void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_TIMEOUT |
WL_EXIT_ON_PM_DEATH,
secs * 1000L + microsecs /
1000,
WAIT_EVENT_RECOVERY_APPLY_DELAY);
@@ -6474,11 +6473,11 @@ StartupXLOG(void)
}
/*
- * Take ownership of the wakeup latch if we're going to sleep during
- * recovery.
+ * Advertise our latch that other processes can use to wake us up
+ * if we're going to sleep during recovery.
*/
if (ArchiveRecoveryRequested)
- OwnLatch(&XLogCtl->recoveryWakeupLatch);
+ XLogCtl->recoveryWakeupLatch = &MyProc->procLatch;
/* Set up XLOG reader facility */
MemSet(&private, 0, sizeof(XLogPageReadPrivate));
@@ -7488,13 +7487,6 @@ StartupXLOG(void)
if (InRecovery)
ResetUnloggedRelations(UNLOGGED_RELATION_INIT);
- /*
- * We don't need the latch anymore. It's not strictly necessary to
disown
- * it, but let's do it for the sake of tidiness.
- */
- if (ArchiveRecoveryRequested)
- DisownLatch(&XLogCtl->recoveryWakeupLatch);
-
/*
* We are now done reading the xlog from stream. Turn off streaming
* recovery to force fetching the files (which would be required at end
of
@@ -12244,12 +12236,12 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool
randAccess,
wait_time =
wal_retrieve_retry_interval -
(secs * 1000 + usecs /
1000);
- (void)
WaitLatch(&XLogCtl->recoveryWakeupLatch,
+ (void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_TIMEOUT |
WL_EXIT_ON_PM_DEATH,
wait_time,
WAIT_EVENT_RECOVERY_RETRIEVE_RETRY_INTERVAL);
-
ResetLatch(&XLogCtl->recoveryWakeupLatch);
+ ResetLatch(MyLatch);
now = GetCurrentTimestamp();
}
last_fail_time = now;
@@ -12500,11 +12492,11 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool
randAccess,
* to react to a trigger file promptly
and to check if the
* WAL receiver is still active.
*/
- (void)
WaitLatch(&XLogCtl->recoveryWakeupLatch,
+ (void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_TIMEOUT |
WL_EXIT_ON_PM_DEATH,
5000L,
WAIT_EVENT_RECOVERY_WAL_STREAM);
-
ResetLatch(&XLogCtl->recoveryWakeupLatch);
+ ResetLatch(MyLatch);
break;
}
@@ -12676,7 +12668,7 @@ CheckPromoteSignal(void)
void
WakeupRecovery(void)
{
- SetLatch(&XLogCtl->recoveryWakeupLatch);
+ SetLatch(XLogCtl->recoveryWakeupLatch);
}
/*
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index 64af7b8707..eab9c8c4ed 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -37,7 +37,6 @@
/*
* Flags set by interrupt handlers for later service in the redo loop.
*/
-static volatile sig_atomic_t got_SIGHUP = false;
static volatile sig_atomic_t shutdown_requested = false;
static volatile sig_atomic_t promote_signaled = false;
@@ -49,7 +48,6 @@ static volatile sig_atomic_t in_restore_command = false;
/* Signal handlers */
static void StartupProcTriggerHandler(SIGNAL_ARGS);
-static void StartupProcSigHupHandler(SIGNAL_ARGS);
/* --------------------------------
@@ -64,19 +62,7 @@ StartupProcTriggerHandler(SIGNAL_ARGS)
int save_errno = errno;
promote_signaled = true;
- WakeupRecovery();
-
- errno = save_errno;
-}
-
-/* SIGHUP: set flag to re-read config file at next convenient time */
-static void
-StartupProcSigHupHandler(SIGNAL_ARGS)
-{
- int save_errno = errno;
-
- got_SIGHUP = true;
- WakeupRecovery();
+ SetLatch(MyLatch);
errno = save_errno;
}
@@ -91,7 +77,7 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
proc_exit(1);
else
shutdown_requested = true;
- WakeupRecovery();
+ SetLatch(MyLatch);
errno = save_errno;
}
@@ -137,9 +123,9 @@ HandleStartupProcInterrupts(void)
/*
* Process any requests or signals received recently.
*/
- if (got_SIGHUP)
+ if (ConfigReloadPending)
{
- got_SIGHUP = false;
+ ConfigReloadPending = false;
StartupRereadConfig();
}
@@ -172,7 +158,7 @@ StartupProcessMain(void)
/*
* Properly accept or ignore signals the postmaster might send us.
*/
- pqsignal(SIGHUP, StartupProcSigHupHandler); /* reload config file */
+ pqsignal(SIGHUP, SignalHandlerForConfigReload); /* reload config file */
pqsignal(SIGINT, SIG_IGN); /* ignore query cancel */
pqsignal(SIGTERM, StartupProcShutdownHandler); /* request shutdown */
/* SIGQUIT handler was already set up by InitPostmasterChild */
