public inbox for [email protected]
help / color / mirror / Atom feedUse standard SIGHUP and SIGTERM handlers in autoprewarm module
38+ messages / 5 participants
[nested] [flat]
* Use standard SIGHUP and SIGTERM handlers in autoprewarm module
@ 2020-10-05 10:45 Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Bharath Rupireddy @ 2020-10-05 10:45 UTC (permalink / raw)
To: pgsql-hackers
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] apw_sigterm_handler() and apw_sighup_handler() use MyProc->procLatch
if (MyProc)
SetLatch(&MyProc->procLatch);
where as standard handlers use MyLatch
SetLatch(MyLatch);
Both MyProc->procLatch and MyLatch point to same, see comment from global.c
/*
* MyLatch points to the latch that should be used for signal handling by
the
* current process. It will either point to a process local latch if the
* current process does not have a PGPROC entry in that moment, or to
* PGPROC->procLatch if it has.
*Thus it can always be used in signal handlers, * without checking for its
existence.*
*/
struct Latch *MyLatch;
(gdb) p MyProc->procLatch
$6 = {is_set = 0, is_shared = true, owner_pid = 1448807}
(gdb) p MyLatch
*$7 = (struct Latch *) 0x7fcacc6d902c*
(gdb) p &MyProc->procLatch
*$8 = (Latch *) 0x7fcacc6d902c*
(gdb) p *MyLatch
$9 = {is_set = 0, is_shared = true, owner_pid = 1448807}
[2] commit 1e53fe0e70f610c34f4c9e770d108cd94151342c
Author: Robert Haas <[email protected]>
Date: 2019-12-17 13:03:57 -0500
Use PostgresSigHupHandler in more places.
There seems to be no reason for every background process to have
its own flag indicating that a config-file reload is needed.
Instead, let's just use ConfigFilePending for that purpose
everywhere.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
[application/x-patch] v1-Use-Standard-SIGTERM-SIGHUP-Handlers-In-AutoPrewarm-Module.patch (2.8K, ../../CALj2ACXPorUqePswDtOeM_s82v9RW32E1fYmOPZ5NuE+TWKj_A@mail.gmail.com/3-v1-Use-Standard-SIGTERM-SIGHUP-Handlers-In-AutoPrewarm-Module.patch)
download | inline diff:
From 7659d36c227fb8193bc76c03f8f215f0a3ac3bb5 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Sat, 3 Oct 2020 09:40:05 +0530
Subject: [PATCH v1] Use Standard SIGTERM,SIGHUP Handlers In AutoPrewarm
Replace apw_sigterm_handler and apw_sighup_handler with standard
signal handlers SignalHandlerForShutdownRequest and
SignalHandlerForConfigReload.
---
contrib/pg_prewarm/autoprewarm.c | 49 ++++----------------------------
1 file changed, 6 insertions(+), 43 deletions(-)
diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index c32ddc56fd..93e526ef62 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -35,6 +35,7 @@
#include "miscadmin.h"
#include "pgstat.h"
#include "postmaster/bgworker.h"
+#include "postmaster/interrupt.h"
#include "storage/buf_internals.h"
#include "storage/dsm.h"
#include "storage/ipc.h"
@@ -94,12 +95,6 @@ static void apw_start_database_worker(void);
static bool apw_init_shmem(void);
static void apw_detach_shmem(int code, Datum arg);
static int apw_compare_blockinfo(const void *p, const void *q);
-static void apw_sigterm_handler(SIGNAL_ARGS);
-static void apw_sighup_handler(SIGNAL_ARGS);
-
-/* Flags set by signal handlers */
-static volatile sig_atomic_t got_sigterm = false;
-static volatile sig_atomic_t got_sighup = false;
/* Pointer to shared-memory state. */
static AutoPrewarmSharedState *apw_state = NULL;
@@ -161,8 +156,8 @@ autoprewarm_main(Datum main_arg)
TimestampTz last_dump_time = 0;
/* Establish signal handlers; once that's done, unblock signals. */
- pqsignal(SIGTERM, apw_sigterm_handler);
- pqsignal(SIGHUP, apw_sighup_handler);
+ pqsignal(SIGTERM, SignalHandlerForShutdownRequest);
+ pqsignal(SIGHUP, SignalHandlerForConfigReload);
pqsignal(SIGUSR1, procsignal_sigusr1_handler);
BackgroundWorkerUnblockSignals();
@@ -206,12 +201,12 @@ autoprewarm_main(Datum main_arg)
}
/* Periodically dump buffers until terminated. */
- while (!got_sigterm)
+ while (!ShutdownRequestPending)
{
/* In case of a SIGHUP, just reload the configuration. */
- if (got_sighup)
+ if (ConfigReloadPending)
{
- got_sighup = false;
+ ConfigReloadPending = false;
ProcessConfigFile(PGC_SIGHUP);
}
@@ -895,35 +890,3 @@ apw_compare_blockinfo(const void *p, const void *q)
return 0;
}
-
-/*
- * Signal handler for SIGTERM
- */
-static void
-apw_sigterm_handler(SIGNAL_ARGS)
-{
- int save_errno = errno;
-
- got_sigterm = true;
-
- if (MyProc)
- SetLatch(&MyProc->procLatch);
-
- errno = save_errno;
-}
-
-/*
- * Signal handler for SIGHUP
- */
-static void
-apw_sighup_handler(SIGNAL_ARGS)
-{
- int save_errno = errno;
-
- got_sighup = true;
-
- if (MyProc)
- SetLatch(&MyProc->procLatch);
-
- errno = save_errno;
-}
--
2.25.1
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
@ 2020-10-05 14:34 ` Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:53 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Michael Paquier <[email protected]>
0 siblings, 2 replies; 38+ messages in thread
From: Fujii Masao @ 2020-10-05 14:34 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; pgsql-hackers
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 */
Attachments:
[text/plain] make_startup_use_general_shared_latch.patch (5.4K, ../../[email protected]/2-make_startup_use_general_shared_latch.patch)
download | inline diff:
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 */
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
@ 2020-10-05 16:18 ` Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
1 sibling, 1 reply; 38+ messages in thread
From: Bharath Rupireddy @ 2020-10-05 16:18 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: pgsql-hackers
On Mon, Oct 5, 2020 at 8:04 PM Fujii Masao <[email protected]> wrote:
>
> 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.
>
Thanks.
>
> 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?
>
I'm not quite sure whether it breaks something or not. I see that
WakeupRecovery() with XLogCtl->recoveryWakeupLatch latch from the
startup process is also being used in the walreceiver process. I may
be wrong, but have some concern if the behaviour is different in case
of EXEC_BACKEND and Windows?
Another concern is that we are always using
XLogCtl->recoveryWakeupLatch in shared mode, this makes sense as this
latch is also being used in walrecevier. But sometimes, MyLatch is
created in non-shared mode as well(see InitLatch(MyLatch)).
Others may have better thoughts though.
>
> Probably we can also replace sigHupHandler() in syslogger.c with
> SignalHandlerForConfigReload()? This would be separate patch, though.
>
+1 to replace sigHupHandler() with SignalHandlerForConfigReload() as
the latch and the functionality are pretty much the same.
WalReceiverMai(): I think we can also replace WalRcvShutdownHandler()
with SignalHandlerForShutdownRequest() because walrcv->latch point to
&MyProc->procLatch which in turn point to MyLatch.
Thoughts? If okay, we can combine these into a single patch. I will
post an updated patch soon.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
@ 2020-10-06 05:50 ` Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Fujii Masao @ 2020-10-06 05:50 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: pgsql-hackers
On 2020/10/06 1:18, Bharath Rupireddy wrote:
> On Mon, Oct 5, 2020 at 8:04 PM Fujii Masao <[email protected]> wrote:
>>
>> 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.
>>
>
> Thanks.
>
>>
>> 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?
>>
>
> I'm not quite sure whether it breaks something or not. I see that
> WakeupRecovery() with XLogCtl->recoveryWakeupLatch latch from the
> startup process is also being used in the walreceiver process. I may
> be wrong, but have some concern if the behaviour is different in case
> of EXEC_BACKEND and Windows?
Unless I'm wrong, regarding MyLatch, the behavior is not different
whether in EXEC_BACKEND or not. In both cases, SwitchToSharedLatch()
is called and MyLatch is set to the shared latch, i.e., MyProc->procLatch.
>
> Another concern is that we are always using
> XLogCtl->recoveryWakeupLatch in shared mode, this makes sense as this
> latch is also being used in walrecevier. But sometimes, MyLatch is
> created in non-shared mode as well(see InitLatch(MyLatch)).
Yes, and then the startup process calls SwitchToSharedLatch() and
repoint MyLatch to the shared one.
>
> Others may have better thoughts though.
Okay, I will reconsider the patch and post it separately later if necessary.
>
>>
>> Probably we can also replace sigHupHandler() in syslogger.c with
>> SignalHandlerForConfigReload()? This would be separate patch, though.
>>
>
> +1 to replace sigHupHandler() with SignalHandlerForConfigReload() as
> the latch and the functionality are pretty much the same.
>
> WalReceiverMai(): I think we can also replace WalRcvShutdownHandler()
> with SignalHandlerForShutdownRequest() because walrcv->latch point to
> &MyProc->procLatch which in turn point to MyLatch.
>
> Thoughts? If okay, we can combine these into a single patch. I will
> post an updated patch soon.
+1 Or it's also ok to make each patch separately.
Anyway, thanks!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
@ 2020-10-06 06:11 ` Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Bharath Rupireddy @ 2020-10-06 06:11 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: pgsql-hackers
On Tue, Oct 6, 2020 at 11:20 AM Fujii Masao <[email protected]> wrote:
>
> >>
> >> Probably we can also replace sigHupHandler() in syslogger.c with
> >> SignalHandlerForConfigReload()? This would be separate patch, though.
> >>
> >
> > +1 to replace sigHupHandler() with SignalHandlerForConfigReload() as
> > the latch and the functionality are pretty much the same.
> >
> > WalReceiverMai(): I think we can also replace WalRcvShutdownHandler()
> > with SignalHandlerForShutdownRequest() because walrcv->latch point to
> > &MyProc->procLatch which in turn point to MyLatch.
> >
> > Thoughts? If okay, we can combine these into a single patch. I will
> > post an updated patch soon.
>
> +1 Or it's also ok to make each patch separately.
> Anyway, thanks!
>
Thanks. +1 to have separate patches. I will post two separate patches
for autoprewarm and WalRcvShutdownHandler for further review. The
other two patches can be for startup.c and syslogger.c.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
@ 2020-10-07 02:30 ` Bharath Rupireddy <[email protected]>
2020-10-07 12:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-29 06:21 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
0 siblings, 2 replies; 38+ messages in thread
From: Bharath Rupireddy @ 2020-10-07 02:30 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: pgsql-hackers
On Tue, Oct 6, 2020 at 11:41 AM Bharath Rupireddy
<[email protected]> wrote:
>
> On Tue, Oct 6, 2020 at 11:20 AM Fujii Masao <[email protected]> wrote:
> >
> > +1 Or it's also ok to make each patch separately.
> > Anyway, thanks!
> >
>
> Thanks. +1 to have separate patches. I will post two separate patches
> for autoprewarm and WalRcvShutdownHandler for further review. The
> other two patches can be for startup.c and syslogger.c.
>
I'm attaching all the 4 patches here together.
1. v1-use-standard-SIGHUP-and-SIGTERM-handlers-in-autoprewarm-process.patch
-- This is the patch initially sent in this mail by me, I just
renamed it.
2. v1-use-MyLatch-and-standard-SIGHUP-handler-in-startup-process.patch
-- This is the patch proposed by Fuji Masao, I just renamed it.
3. v1-use-standard-SIGHUP-hanlder-in-syslogger-process.patch
4. v1-use-standard-SIGTERM-handler-in-walreceiver-process.patch
Please consider these patches for further review.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
[application/x-patch] v1-use-standard-SIGHUP-and-SIGTERM-handlers-in-autoprewarm-process.patch (2.8K, ../../CALj2ACX0JkSM2BhiLog1xP9x2tsdjVdL8U-=OABf2YzYS+ywpQ@mail.gmail.com/2-v1-use-standard-SIGHUP-and-SIGTERM-handlers-in-autoprewarm-process.patch)
download | inline diff:
From b9d77b6e84d3eee87693893c1687f120ee3db68b Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Wed, 7 Oct 2020 07:12:57 +0530
Subject: [PATCH v1] autoprewarm use standard SIGHUP and SIGTERM handlers
Replace apw_sigterm_handler() and apw_sighup_handler() with standard
SignalHandlerForShutdownRequest() and SignalHandlerForConfigReload()
respectively.
---
contrib/pg_prewarm/autoprewarm.c | 49 ++++----------------------------
1 file changed, 6 insertions(+), 43 deletions(-)
diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index c32ddc56fd..93e526ef62 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -35,6 +35,7 @@
#include "miscadmin.h"
#include "pgstat.h"
#include "postmaster/bgworker.h"
+#include "postmaster/interrupt.h"
#include "storage/buf_internals.h"
#include "storage/dsm.h"
#include "storage/ipc.h"
@@ -94,12 +95,6 @@ static void apw_start_database_worker(void);
static bool apw_init_shmem(void);
static void apw_detach_shmem(int code, Datum arg);
static int apw_compare_blockinfo(const void *p, const void *q);
-static void apw_sigterm_handler(SIGNAL_ARGS);
-static void apw_sighup_handler(SIGNAL_ARGS);
-
-/* Flags set by signal handlers */
-static volatile sig_atomic_t got_sigterm = false;
-static volatile sig_atomic_t got_sighup = false;
/* Pointer to shared-memory state. */
static AutoPrewarmSharedState *apw_state = NULL;
@@ -161,8 +156,8 @@ autoprewarm_main(Datum main_arg)
TimestampTz last_dump_time = 0;
/* Establish signal handlers; once that's done, unblock signals. */
- pqsignal(SIGTERM, apw_sigterm_handler);
- pqsignal(SIGHUP, apw_sighup_handler);
+ pqsignal(SIGTERM, SignalHandlerForShutdownRequest);
+ pqsignal(SIGHUP, SignalHandlerForConfigReload);
pqsignal(SIGUSR1, procsignal_sigusr1_handler);
BackgroundWorkerUnblockSignals();
@@ -206,12 +201,12 @@ autoprewarm_main(Datum main_arg)
}
/* Periodically dump buffers until terminated. */
- while (!got_sigterm)
+ while (!ShutdownRequestPending)
{
/* In case of a SIGHUP, just reload the configuration. */
- if (got_sighup)
+ if (ConfigReloadPending)
{
- got_sighup = false;
+ ConfigReloadPending = false;
ProcessConfigFile(PGC_SIGHUP);
}
@@ -895,35 +890,3 @@ apw_compare_blockinfo(const void *p, const void *q)
return 0;
}
-
-/*
- * Signal handler for SIGTERM
- */
-static void
-apw_sigterm_handler(SIGNAL_ARGS)
-{
- int save_errno = errno;
-
- got_sigterm = true;
-
- if (MyProc)
- SetLatch(&MyProc->procLatch);
-
- errno = save_errno;
-}
-
-/*
- * Signal handler for SIGHUP
- */
-static void
-apw_sighup_handler(SIGNAL_ARGS)
-{
- int save_errno = errno;
-
- got_sighup = true;
-
- if (MyProc)
- SetLatch(&MyProc->procLatch);
-
- errno = save_errno;
-}
--
2.25.1
[application/x-patch] v1-use-MyLatch-and-standard-SIGHUP-handler-in-startup-process.patch (6.1K, ../../CALj2ACX0JkSM2BhiLog1xP9x2tsdjVdL8U-=OABf2YzYS+ywpQ@mail.gmail.com/3-v1-use-MyLatch-and-standard-SIGHUP-handler-in-startup-process.patch)
download | inline diff:
From 50d718fec6ade8c101d466ee71f0ab30ed020182 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Wed, 7 Oct 2020 07:17:49 +0530
Subject: [PATCH v1] startup use MyLatch and standard SIGHUP handler
Remove a separate shared latch with MyLatch which is also a shared
latch in startup process. This change can let startup process use
standard SIGHUP handler SignalHandlerForConfigReload() instead of
StartupProcSigHupHandler().
---
src/backend/access/transam/xlog.c | 30 +++++++++++-------------------
src/backend/postmaster/startup.c | 24 +++++-------------------
2 files changed, 16 insertions(+), 38 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 52a67b1170..c211d66994 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
@@ -12242,12 +12234,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;
@@ -12498,11 +12490,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;
}
@@ -12674,7 +12666,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 */
--
2.25.1
[application/x-patch] v1-use-standard-SIGHUP-hanlder-in-syslogger-process.patch (2.4K, ../../CALj2ACX0JkSM2BhiLog1xP9x2tsdjVdL8U-=OABf2YzYS+ywpQ@mail.gmail.com/4-v1-use-standard-SIGHUP-hanlder-in-syslogger-process.patch)
download | inline diff:
From a521ed1c62df788a7b613b7ee669d58844951668 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Wed, 7 Oct 2020 07:34:38 +0530
Subject: [PATCH v1] syslogger - use standard SIGHUP handler
Replace sigHupHanlder() with standard SignalHandlerForConfigReload().
---
src/backend/postmaster/syslogger.c | 21 ++++-----------------
1 file changed, 4 insertions(+), 17 deletions(-)
diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c
index ffcb54968f..faa82ec481 100644
--- a/src/backend/postmaster/syslogger.c
+++ b/src/backend/postmaster/syslogger.c
@@ -39,6 +39,7 @@
#include "pgstat.h"
#include "pgtime.h"
#include "postmaster/fork_process.h"
+#include "postmaster/interrupt.h"
#include "postmaster/postmaster.h"
#include "postmaster/syslogger.h"
#include "storage/dsm.h"
@@ -123,7 +124,6 @@ static CRITICAL_SECTION sysloggerSection;
/*
* Flags set by interrupt handlers for later service in the main loop.
*/
-static volatile sig_atomic_t got_SIGHUP = false;
static volatile sig_atomic_t rotation_requested = false;
@@ -144,7 +144,6 @@ static unsigned int __stdcall pipeThread(void *arg);
static void logfile_rotate(bool time_based_rotation, int size_rotation_for);
static char *logfile_getname(pg_time_t timestamp, const char *suffix);
static void set_next_rotation_time(void);
-static void sigHupHandler(SIGNAL_ARGS);
static void sigUsr1Handler(SIGNAL_ARGS);
static void update_metainfo_datafile(void);
@@ -240,7 +239,7 @@ SysLoggerMain(int argc, char *argv[])
* broken backends...
*/
- pqsignal(SIGHUP, sigHupHandler); /* set flag to read config file */
+ pqsignal(SIGHUP, SignalHandlerForConfigReload); /* set flag to read config file */
pqsignal(SIGINT, SIG_IGN);
pqsignal(SIGTERM, SIG_IGN);
pqsignal(SIGQUIT, SIG_IGN);
@@ -324,9 +323,9 @@ SysLoggerMain(int argc, char *argv[])
/*
* Process any requests or signals received recently.
*/
- if (got_SIGHUP)
+ if (ConfigReloadPending)
{
- got_SIGHUP = false;
+ ConfigReloadPending = false;
ProcessConfigFile(PGC_SIGHUP);
/*
@@ -1553,18 +1552,6 @@ RemoveLogrotateSignalFiles(void)
unlink(LOGROTATE_SIGNAL_FILE);
}
-/* SIGHUP: set flag to reload config file */
-static void
-sigHupHandler(SIGNAL_ARGS)
-{
- int save_errno = errno;
-
- got_SIGHUP = true;
- SetLatch(MyLatch);
-
- errno = save_errno;
-}
-
/* SIGUSR1: set flag to rotate logfile */
static void
sigUsr1Handler(SIGNAL_ARGS)
--
2.25.1
[application/x-patch] v1-use-standard-SIGTERM-handler-in-walreceiver-process.patch (2.2K, ../../CALj2ACX0JkSM2BhiLog1xP9x2tsdjVdL8U-=OABf2YzYS+ywpQ@mail.gmail.com/5-v1-use-standard-SIGTERM-handler-in-walreceiver-process.patch)
download | inline diff:
From 45c36aab4a2b1c66967320688ffc9ee916032abc Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Wed, 7 Oct 2020 07:09:35 +0530
Subject: [PATCH v1] walreceiver use standard SIGTERM handler
Replace WalRcvShutdownHandler() with standard SignalHandlerForShutdownRequest().
---
src/backend/replication/walreceiver.c | 22 ++--------------------
1 file changed, 2 insertions(+), 20 deletions(-)
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index bb1d44ccb7..65f933898d 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -109,7 +109,6 @@ static XLogSegNo recvSegNo = 0;
* main loop.
*/
static volatile sig_atomic_t got_SIGHUP = false;
-static volatile sig_atomic_t got_SIGTERM = false;
/*
* LogstreamResult indicates the byte positions that we have already
@@ -137,8 +136,6 @@ static void ProcessWalSndrMessage(XLogRecPtr walEnd, TimestampTz sendTime);
/* Signal handlers */
static void WalRcvSigHupHandler(SIGNAL_ARGS);
-static void WalRcvShutdownHandler(SIGNAL_ARGS);
-
/*
* Process any interrupts the walreceiver process may have received.
@@ -164,7 +161,7 @@ ProcessWalRcvInterrupts(void)
*/
CHECK_FOR_INTERRUPTS();
- if (got_SIGTERM)
+ if (ShutdownRequestPending)
{
ereport(FATAL,
(errcode(ERRCODE_ADMIN_SHUTDOWN),
@@ -269,7 +266,7 @@ WalReceiverMain(void)
/* Properly accept or ignore signals the postmaster might send us */
pqsignal(SIGHUP, WalRcvSigHupHandler); /* set flag to read config file */
pqsignal(SIGINT, SIG_IGN);
- pqsignal(SIGTERM, WalRcvShutdownHandler); /* request shutdown */
+ pqsignal(SIGTERM, SignalHandlerForShutdownRequest); /* request shutdown */
/* SIGQUIT handler was already set up by InitPostmasterChild */
pqsignal(SIGALRM, SIG_IGN);
pqsignal(SIGPIPE, SIG_IGN);
@@ -813,21 +810,6 @@ WalRcvSigHupHandler(SIGNAL_ARGS)
got_SIGHUP = true;
}
-
-/* SIGTERM: set flag for ProcessWalRcvInterrupts */
-static void
-WalRcvShutdownHandler(SIGNAL_ARGS)
-{
- int save_errno = errno;
-
- got_SIGTERM = true;
-
- if (WalRcv->latch)
- SetLatch(WalRcv->latch);
-
- errno = save_errno;
-}
-
/*
* Accept the message from XLOG stream, and process it.
*/
--
2.25.1
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
@ 2020-10-07 12:38 ` Bharath Rupireddy <[email protected]>
2020-10-23 04:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Craig Ringer <[email protected]>
1 sibling, 1 reply; 38+ messages in thread
From: Bharath Rupireddy @ 2020-10-07 12:38 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: pgsql-hackers
On Wed, Oct 7, 2020 at 8:00 AM Bharath Rupireddy
<[email protected]> wrote:
>
> On Tue, Oct 6, 2020 at 11:41 AM Bharath Rupireddy
> <[email protected]> wrote:
> >
> > On Tue, Oct 6, 2020 at 11:20 AM Fujii Masao <[email protected]> wrote:
> > >
> > > +1 Or it's also ok to make each patch separately.
> > > Anyway, thanks!
> > >
> >
> > Thanks. +1 to have separate patches. I will post two separate patches
> > for autoprewarm and WalRcvShutdownHandler for further review. The
> > other two patches can be for startup.c and syslogger.c.
> >
>
> I'm attaching all the 4 patches here together.
>
> 1. v1-use-standard-SIGHUP-and-SIGTERM-handlers-in-autoprewarm-process.patch
> -- This is the patch initially sent in this mail by me, I just
> renamed it.
> 2. v1-use-MyLatch-and-standard-SIGHUP-handler-in-startup-process.patch
> -- This is the patch proposed by Fuji Masao, I just renamed it.
> 3. v1-use-standard-SIGHUP-hanlder-in-syslogger-process.patch
> 4. v1-use-standard-SIGTERM-handler-in-walreceiver-process.patch
>
> Please consider these patches for further review.
>
Added this to the commitfest for further review.
https://commitfest.postgresql.org/30/2756/
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 12:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
@ 2020-10-23 04:06 ` Craig Ringer <[email protected]>
2020-11-17 12:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Craig Ringer @ 2020-10-23 04:06 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: Fujii Masao <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>
On Wed, Oct 7, 2020 at 8:39 PM Bharath Rupireddy <
[email protected]> wrote:
> On Wed, Oct 7, 2020 at 8:00 AM Bharath Rupireddy
> <[email protected]> wrote:
> >
> > On Tue, Oct 6, 2020 at 11:41 AM Bharath Rupireddy
> > <[email protected]> wrote:
> > >
> > > On Tue, Oct 6, 2020 at 11:20 AM Fujii Masao <
> [email protected]> wrote:
> > > >
> > > > +1 Or it's also ok to make each patch separately.
> > > > Anyway, thanks!
> > > >
> > >
> > > Thanks. +1 to have separate patches. I will post two separate patches
> > > for autoprewarm and WalRcvShutdownHandler for further review. The
> > > other two patches can be for startup.c and syslogger.c.
>
[ CC'd Robert Haas since he's the main name in interrupt.c,
test_shm_mq/worker.c, ]
src/test/modules/test_shm_mq/worker.c appears to do the right thing the
wrong way - it has its own custom handler instead of using die() or
SignalHandlerForShutdownRequest().
In contrast src/test/modules/worker_spi/worker_spi.c looks plain wrong.
Especially since it's quoted as an example of how to do things right. It
won't respond to SIGTERM at all while it's executing a query from its
queue, no matter how long that query takes or whether it blocks. It can
inhibit even postmaster shutdown as a result.
I was going to lob off a quick patch to fix this by making both use
quickdie() for SIGQUIT and die() for SIGTERM, but after reading for a bit
I'm no longer sure what the right choice even is. I'd welcome some opinions.
The problem is that but interrupt.c and interrupt.h actually define and
recommend different and simpler handlers for these jobs - ones that don't
actually work properly for code that calls into Pg core and might rely on
CHECK_FOR_INTERRUPTS(), InterruptsPending and ProcDiePending to properly
respect SIGTERM.
And to add to the confusion the bgworker infra adds its own different
default SIGTERM handler bgworker_die() that's weirdly in-between the
interrupt.c and postgres.c signal handling.
So I'm no longer sure how the example code should even be fixed. I'm not
convinced everyone using die() and quickdie() is good given they currently
seem to be assumed to be mainly for user backends. Maybe wwe should move
them to interrupt.c along with CHECK_FOR_INTERRUPTS(), ProcessInterrupts,
etc and document them as for all database-connected or shmem-connected
backends to use.
So in the medium term, interrupt.c's SignalHandlerForShutdownRequest() and
SignalHandlerForCrashExit() should be combined with die() and quickdie(),
integrating properly with CHECK_FOR_INTERRUPTS(), ProcessInterrupts(), etc.
We can add a hook we call before we proc_exit() in response to
ProcDiePending so backends can choose to mask it if there's a period during
which they wish to defer responding to SIGTERM, but by default everything
will respect SIGTERM -> die() sets ProcDiePending -> CHECK_FOR_INTERRUPTS()
-> ProcessInterrupts() -> proc_exit() . interrupt.c's
SignalHandlerForCrashExit() and SignalHandlerForShutdownRequest() become
deprecated/legacy. We add a separate temporary handler that's installed by
init.c for early SIGQUIT handling but document it as to be replaced after
backends start properly. We'd delete the bgw-specific signal handlers and
install die() and procdie() instead during StartBackgroundWorker - at least
if the bgw is connecting to shmem or a database. interrupt.c's
HandleMainLoopInterrupts() could be static inlined, and adopted in the
bgworker examples and all the other places that currently do
ConfigReloadPending / ProcessConfigFile() etc themselves.
It wouldn't be a clean sweep of consistent signal handling, given all the
funky stuff we have in the checkpointer, walsender, etc. But I think it
might help...
(And maybe I could even combine the various am_foo and is_bar globals we
use to identify different sorts of backend, while doing such cleanups).
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 12:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-23 04:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Craig Ringer <[email protected]>
@ 2020-11-17 12:18 ` Bharath Rupireddy <[email protected]>
2020-11-18 12:22 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Bharath Rupireddy @ 2020-11-17 12:18 UTC (permalink / raw)
To: Craig Ringer <[email protected]>; +Cc: Fujii Masao <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>
Thanks Craig!
On Fri, Oct 23, 2020 at 9:37 AM Craig Ringer
<[email protected]> wrote:
>
> src/test/modules/test_shm_mq/worker.c appears to do the right thing the wrong way - it has its own custom handler instead of using die() or SignalHandlerForShutdownRequest().
>
handle_sigterm() and die() are similar except that die() has extra
handling(below) for exiting immediately when waiting for input/command
from the client.
/*
* If we're in single user mode, we want to quit immediately - we can't
* rely on latches as they wouldn't work when stdin/stdout is a file.
* Rather ugly, but it's unlikely to be worthwhile to invest much more
* effort just for the benefit of single user mode.
*/
if (DoingCommandRead && whereToSendOutput != DestRemote)
ProcessInterrupts();
Having this extra handling is correct for normal backends as they can
connect directly to clients for reading input commands, the above if
condition may become true and ProcessInterrupts() may be called to
handle the SIGTERM immediately.
Note that DoingCommandRead can never be true in bg workers as it's
being set to true only in normal backend PostgresMain(). If we use
die()(instead of custom SIGTERM handlers such as handle_sigterm()) in
a bg worker/non-backend process, there are no chances that the above
part of the code gets hit and the interrupts are processed
immediately. And also here are the bg worker process that use die()
instead of their own custom handlers: parallel workers, autoprewarm
worker, autovacuum worker, logical replication launcher and apply
worker, wal sender process
I think we can also use die() instead of handle_sigterm() in
test_shm_mq/worker.c.
I attached a patch for this change.
>
> In contrast src/test/modules/worker_spi/worker_spi.c looks plain wrong. Especially since it's quoted as an example of how to do things right. It won't respond to SIGTERM at all while it's executing a query from its queue, no matter how long that query takes or whether it blocks. It can inhibit even postmaster shutdown as a result.
>
Postmaster sends SIGTERM to all children(backends and bgworkers) for
both smart shutdown(wait for children to end their work, then shut
down.) and fast shutdown(rollback active transactions and shutdown
when they are gone.) For immediate shutdown SIGQUIT is sent to
children.(see pmdie()).
For each bg worker(so is for worker_spi.c),
SignalHandlerForCrashExit() is set as SIGQUIT handler in
InitPostmasterChild(), which does nothing but exits immediately. We
can not use quickdie() here because as a bg worker we don't have
to/can not send anything to client. We are good with
SignalHandlerForCrashExit() as with all other bg workers.
Yes, if having worker_spi_sigterm/SignalHandlerForShutdownRequest,
sometimes(as explained above) the worker_spi worker may not respond to
SIGTERM. I think we should be having die() as SIGTERM handler here (as
with normal backend and parallel workers).
Attaching another patch that has replaced custom SIGTERM handler
worker_spi_sigterm() with die() and custom SIGHUP handler
worker_spi_sighup() with standard SignalHandlerForConfigReload().
>
> I was going to lob off a quick patch to fix this by making both use quickdie() for SIGQUIT and die() for SIGTERM, but after reading for a bit I'm no longer sure what the right choice even is. I'd welcome some opinions.
>
We can not use quickdie() here because as a bg worker we don't have
to/can not send anything to client. We are good with
SignalHandlerForCrashExit() as with all other bg workers.
Thoughts?
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
[application/octet-stream] v1-0001-Use-die-instead-of-custom-signal-handler-in-test_shm_mq-worker.patch (2.5K, ../../CALj2ACWWy1YcngpCUn09AsXMfOzwjfNqbVosfoRY0vhhVWhVBw@mail.gmail.com/2-v1-0001-Use-die-instead-of-custom-signal-handler-in-test_shm_mq-worker.patch)
download | inline diff:
From e89b68502d4fa7c84ce637c27a81eb502b51eaff Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Tue, 17 Nov 2020 17:33:25 +0530
Subject: [PATCH v1] Use die() instead of custom signal handler in test_shm_mq
worker
DoingCommandRead can never be true in bg workers as it's being set
to true only in normal backend PostgresMain(). If we use die()
(instead of custom SIGTERM handlers such as handle_sigterm()) in a
bg worker/non-backend process, there are no chances that the
DoinCommandREad part of the code gets hit and the interrupts are
processed immediately. And also here are the bg worker process
that use die() instead of their own custom handlers:
parallel workers, autoprewarm worker, autovacuum worker,
logical replication launcher and apply worker, wal sender process
---
src/test/modules/test_shm_mq/worker.c | 28 +++------------------------
1 file changed, 3 insertions(+), 25 deletions(-)
diff --git a/src/test/modules/test_shm_mq/worker.c b/src/test/modules/test_shm_mq/worker.c
index cb63adebee..3f2e9bf812 100644
--- a/src/test/modules/test_shm_mq/worker.c
+++ b/src/test/modules/test_shm_mq/worker.c
@@ -24,10 +24,10 @@
#include "storage/procarray.h"
#include "storage/shm_mq.h"
#include "storage/shm_toc.h"
+#include "tcop/tcopprot.h"
#include "test_shm_mq.h"
-static void handle_sigterm(SIGNAL_ARGS);
static void attach_to_queues(dsm_segment *seg, shm_toc *toc,
int myworkernumber, shm_mq_handle **inqhp,
shm_mq_handle **outqhp);
@@ -58,10 +58,9 @@ test_shm_mq_main(Datum main_arg)
* Establish signal handlers.
*
* We want CHECK_FOR_INTERRUPTS() to kill off this worker process just as
- * it would a normal user backend. To make that happen, we establish a
- * signal handler that is a stripped-down version of die().
+ * it would a normal user backend. To make that happen, we use die().
*/
- pqsignal(SIGTERM, handle_sigterm);
+ pqsignal(SIGTERM, die);
BackgroundWorkerUnblockSignals();
/*
@@ -196,24 +195,3 @@ copy_messages(shm_mq_handle *inqh, shm_mq_handle *outqh)
break;
}
}
-
-/*
- * When we receive a SIGTERM, we set InterruptPending and ProcDiePending just
- * like a normal backend. The next CHECK_FOR_INTERRUPTS() will do the right
- * thing.
- */
-static void
-handle_sigterm(SIGNAL_ARGS)
-{
- int save_errno = errno;
-
- SetLatch(MyLatch);
-
- if (!proc_exit_inprogress)
- {
- InterruptPending = true;
- ProcDiePending = true;
- }
-
- errno = save_errno;
-}
--
2.25.1
[application/octet-stream] v1-0001-Use-standard-SIGHUP-handler-and-SIGTERM-handler-die-in-worker_spi.patch (3.0K, ../../CALj2ACWWy1YcngpCUn09AsXMfOzwjfNqbVosfoRY0vhhVWhVBw@mail.gmail.com/3-v1-0001-Use-standard-SIGHUP-handler-and-SIGTERM-handler-die-in-worker_spi.patch)
download | inline diff:
From 6731677f529509abb87825104a5b7bc1df939650 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Tue, 17 Nov 2020 17:22:47 +0530
Subject: [PATCH v1] Use standard SIGHUP handler and SIGTERM handler die in
worker_spi
---
src/test/modules/worker_spi/worker_spi.c | 52 ++++--------------------
1 file changed, 9 insertions(+), 43 deletions(-)
diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index 258237f9bf..71f955206a 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -25,6 +25,7 @@
/* These are always necessary for a bgworker */
#include "miscadmin.h"
#include "postmaster/bgworker.h"
+#include "postmaster/interrupt.h"
#include "storage/ipc.h"
#include "storage/latch.h"
#include "storage/lwlock.h"
@@ -48,10 +49,6 @@ PG_FUNCTION_INFO_V1(worker_spi_launch);
void _PG_init(void);
void worker_spi_main(Datum) pg_attribute_noreturn();
-/* flags set by signal handlers */
-static volatile sig_atomic_t got_sighup = false;
-static volatile sig_atomic_t got_sigterm = false;
-
/* GUC variables */
static int worker_spi_naptime = 10;
static int worker_spi_total_workers = 2;
@@ -64,38 +61,6 @@ typedef struct worktable
const char *name;
} worktable;
-/*
- * Signal handler for SIGTERM
- * Set a flag to let the main loop to terminate, and set our latch to wake
- * it up.
- */
-static void
-worker_spi_sigterm(SIGNAL_ARGS)
-{
- int save_errno = errno;
-
- got_sigterm = true;
- SetLatch(MyLatch);
-
- errno = save_errno;
-}
-
-/*
- * Signal handler for SIGHUP
- * Set a flag to tell the main loop to reread the config file, and set
- * our latch to wake it up.
- */
-static void
-worker_spi_sighup(SIGNAL_ARGS)
-{
- int save_errno = errno;
-
- got_sighup = true;
- SetLatch(MyLatch);
-
- errno = save_errno;
-}
-
/*
* Initialize workspace for a worker process: create the schema if it doesn't
* already exist.
@@ -179,8 +144,8 @@ worker_spi_main(Datum main_arg)
table->name = pstrdup("counted");
/* Establish signal handlers before unblocking signals. */
- pqsignal(SIGHUP, worker_spi_sighup);
- pqsignal(SIGTERM, worker_spi_sigterm);
+ pqsignal(SIGHUP, SignalHandlerForConfigReload);
+ pqsignal(SIGTERM, die);
/* We're now ready to receive signals */
BackgroundWorkerUnblockSignals();
@@ -219,9 +184,10 @@ worker_spi_main(Datum main_arg)
table->name);
/*
- * Main loop: do this until the SIGTERM handler tells us to terminate
+ * Main loop: do this until SIGTERM is received and processed by
+ * ProcessInterrupts.
*/
- while (!got_sigterm)
+ for (;;)
{
int ret;
@@ -242,9 +208,9 @@ worker_spi_main(Datum main_arg)
/*
* In case of a SIGHUP, just reload the configuration.
*/
- if (got_sighup)
+ if (ConfigReloadPending)
{
- got_sighup = false;
+ ConfigReloadPending = false;
ProcessConfigFile(PGC_SIGHUP);
}
@@ -303,7 +269,7 @@ worker_spi_main(Datum main_arg)
pgstat_report_activity(STATE_IDLE, NULL);
}
- proc_exit(1);
+ /* Not reachable */
}
/*
--
2.25.1
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 12:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-23 04:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Craig Ringer <[email protected]>
2020-11-17 12:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
@ 2020-11-18 12:22 ` Fujii Masao <[email protected]>
2020-11-20 10:33 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Fujii Masao @ 2020-11-18 12:22 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; Craig Ringer <[email protected]>; +Cc: pgsql-hackers; Robert Haas <[email protected]>
On 2020/11/17 21:18, Bharath Rupireddy wrote:
> Thanks Craig!
>
> On Fri, Oct 23, 2020 at 9:37 AM Craig Ringer
> <[email protected]> wrote:
>>
>> src/test/modules/test_shm_mq/worker.c appears to do the right thing the wrong way - it has its own custom handler instead of using die() or SignalHandlerForShutdownRequest().
>>
>
> handle_sigterm() and die() are similar except that die() has extra
> handling(below) for exiting immediately when waiting for input/command
> from the client.
> /*
> * If we're in single user mode, we want to quit immediately - we can't
> * rely on latches as they wouldn't work when stdin/stdout is a file.
> * Rather ugly, but it's unlikely to be worthwhile to invest much more
> * effort just for the benefit of single user mode.
> */
> if (DoingCommandRead && whereToSendOutput != DestRemote)
> ProcessInterrupts();
>
> Having this extra handling is correct for normal backends as they can
> connect directly to clients for reading input commands, the above if
> condition may become true and ProcessInterrupts() may be called to
> handle the SIGTERM immediately.
>
> Note that DoingCommandRead can never be true in bg workers as it's
> being set to true only in normal backend PostgresMain(). If we use
> die()(instead of custom SIGTERM handlers such as handle_sigterm()) in
> a bg worker/non-backend process, there are no chances that the above
> part of the code gets hit and the interrupts are processed
> immediately. And also here are the bg worker process that use die()
> instead of their own custom handlers: parallel workers, autoprewarm
> worker, autovacuum worker, logical replication launcher and apply
> worker, wal sender process
>
> I think we can also use die() instead of handle_sigterm() in
> test_shm_mq/worker.c.
>
> I attached a patch for this change.
Thanks for the patch! It looks good to me.
BTW, I tried to find the past discussion about why die() was not used,
but I could not yet.
>
>>
>> In contrast src/test/modules/worker_spi/worker_spi.c looks plain wrong. Especially since it's quoted as an example of how to do things right. It won't respond to SIGTERM at all while it's executing a query from its queue, no matter how long that query takes or whether it blocks. It can inhibit even postmaster shutdown as a result.
>>
>
> Postmaster sends SIGTERM to all children(backends and bgworkers) for
> both smart shutdown(wait for children to end their work, then shut
> down.) and fast shutdown(rollback active transactions and shutdown
> when they are gone.) For immediate shutdown SIGQUIT is sent to
> children.(see pmdie()).
>
> For each bg worker(so is for worker_spi.c),
> SignalHandlerForCrashExit() is set as SIGQUIT handler in
> InitPostmasterChild(), which does nothing but exits immediately. We
> can not use quickdie() here because as a bg worker we don't have
> to/can not send anything to client. We are good with
> SignalHandlerForCrashExit() as with all other bg workers.
>
> Yes, if having worker_spi_sigterm/SignalHandlerForShutdownRequest,
> sometimes(as explained above) the worker_spi worker may not respond to
> SIGTERM. I think we should be having die() as SIGTERM handler here (as
> with normal backend and parallel workers).
>
> Attaching another patch that has replaced custom SIGTERM handler
> worker_spi_sigterm() with die() and custom SIGHUP handler
> worker_spi_sighup() with standard SignalHandlerForConfigReload().
This patch also looks good to me.
>
>>
>> I was going to lob off a quick patch to fix this by making both use quickdie() for SIGQUIT and die() for SIGTERM, but after reading for a bit I'm no longer sure what the right choice even is. I'd welcome some opinions.
>>
>
> We can not use quickdie() here because as a bg worker we don't have
> to/can not send anything to client. We are good with
> SignalHandlerForCrashExit() as with all other bg workers.
>
> Thoughts?
I think you're right.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 12:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-23 04:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Craig Ringer <[email protected]>
2020-11-17 12:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-18 12:22 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
@ 2020-11-20 10:33 ` Bharath Rupireddy <[email protected]>
2020-11-25 09:59 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Bharath Rupireddy @ 2020-11-20 10:33 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: Craig Ringer <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>
On Wed, Nov 18, 2020 at 5:52 PM Fujii Masao <[email protected]>
wrote:
>
> > handle_sigterm() and die() are similar except that die() has extra
> > handling(below) for exiting immediately when waiting for input/command
> > from the client.
> > /*
> > * If we're in single user mode, we want to quit immediately - we
can't
> > * rely on latches as they wouldn't work when stdin/stdout is a
file.
> > * Rather ugly, but it's unlikely to be worthwhile to invest much
more
> > * effort just for the benefit of single user mode.
> > */
> > if (DoingCommandRead && whereToSendOutput != DestRemote)
> > ProcessInterrupts();
> >
> > Having this extra handling is correct for normal backends as they can
> > connect directly to clients for reading input commands, the above if
> > condition may become true and ProcessInterrupts() may be called to
> > handle the SIGTERM immediately.
> >
> > Note that DoingCommandRead can never be true in bg workers as it's
> > being set to true only in normal backend PostgresMain(). If we use
> > die()(instead of custom SIGTERM handlers such as handle_sigterm()) in
> > a bg worker/non-backend process, there are no chances that the above
> > part of the code gets hit and the interrupts are processed
> > immediately. And also here are the bg worker process that use die()
> > instead of their own custom handlers: parallel workers, autoprewarm
> > worker, autovacuum worker, logical replication launcher and apply
> > worker, wal sender process
> >
> > I think we can also use die() instead of handle_sigterm() in
> > test_shm_mq/worker.c.
> >
> > I attached a patch for this change.
>
> Thanks for the patch! It looks good to me.
>
Thanks.
>
> BTW, I tried to find the past discussion about why die() was not used,
> but I could not yet.
>
I think the commit 2505ce0 that altered the comment has something:
handle_sigterm() is stripped down to remove if (DoingCommandRead &&
whereToSendOutput != DestRemote) part from die() since test_shm_mq bg
worker or for that matter any bg worker do not have an equivalent of the
regular backend's command-read loop.
--- a/src/test/modules/test_shm_mq/worker.c
+++ b/src/test/modules/test_shm_mq/worker.c
@@ -60,13 +60,9 @@ test_shm_mq_main(Datum main_arg)
*
* We want CHECK_FOR_INTERRUPTS() to kill off this worker process
just as
* it would a normal user backend. To make that happen, we
establish a
- * signal handler that is a stripped-down version of die(). We
don't have
- * any equivalent of the backend's command-read loop, where
interrupts can
- * be processed immediately, so make sure ImmediateInterruptOK is
turned
- * off.
+ * signal handler that is a stripped-down version of die().
*/
pqsignal(SIGTERM, handle_sigterm);
- ImmediateInterruptOK = false;
BackgroundWorkerUnblockSignals();
>
> > Attaching another patch that has replaced custom SIGTERM handler
> > worker_spi_sigterm() with die() and custom SIGHUP handler
> > worker_spi_sighup() with standard SignalHandlerForConfigReload().
>
> This patch also looks good to me.
>
Thanks.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 12:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-23 04:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Craig Ringer <[email protected]>
2020-11-17 12:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-18 12:22 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-20 10:33 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
@ 2020-11-25 09:59 ` Fujii Masao <[email protected]>
2020-11-25 14:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Fujii Masao @ 2020-11-25 09:59 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: Craig Ringer <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>
On 2020/11/20 19:33, Bharath Rupireddy wrote:
> On Wed, Nov 18, 2020 at 5:52 PM Fujii Masao <[email protected] <mailto:[email protected]>> wrote:
> >
> > > handle_sigterm() and die() are similar except that die() has extra
> > > handling(below) for exiting immediately when waiting for input/command
> > > from the client.
> > > /*
> > > * If we're in single user mode, we want to quit immediately - we can't
> > > * rely on latches as they wouldn't work when stdin/stdout is a file.
> > > * Rather ugly, but it's unlikely to be worthwhile to invest much more
> > > * effort just for the benefit of single user mode.
> > > */
> > > if (DoingCommandRead && whereToSendOutput != DestRemote)
> > > ProcessInterrupts();
> > >
> > > Having this extra handling is correct for normal backends as they can
> > > connect directly to clients for reading input commands, the above if
> > > condition may become true and ProcessInterrupts() may be called to
> > > handle the SIGTERM immediately.
> > >
> > > Note that DoingCommandRead can never be true in bg workers as it's
> > > being set to true only in normal backend PostgresMain(). If we use
> > > die()(instead of custom SIGTERM handlers such as handle_sigterm()) in
> > > a bg worker/non-backend process, there are no chances that the above
> > > part of the code gets hit and the interrupts are processed
> > > immediately. And also here are the bg worker process that use die()
> > > instead of their own custom handlers: parallel workers, autoprewarm
> > > worker, autovacuum worker, logical replication launcher and apply
> > > worker, wal sender process
> > >
> > > I think we can also use die() instead of handle_sigterm() in
> > > test_shm_mq/worker.c.
> > >
> > > I attached a patch for this change.
> >
> > Thanks for the patch! It looks good to me.
> >
>
> Thanks.
When I read the patch again, I found that, with the patch, the shutdown
of worker_spi causes to report the following FATAL message.
FATAL: terminating connection due to administrator command
Isn't this message confusing because it's not a connection? If so,
we need to update ProcessInterrupts() so that the proper message is
reported like other bgworkers do.
BTW, though this is not directly related to this topic, it's strange to me
that the normal shutdown of bgworker causes to emit FATAL message
and the message "background worker ... exited with exit code 1"
at the normal shutdown. I've heard sometimes that users coplained that
it's confusing to report that message for logical replication launcher
at the normal shutdown. Maybe the mechanism to shutdown bgworker
normally seems to have not been implemented well yet.
Without the patch, since worker_spi cannot handle the shutdown request
promptly while it's executing the backend code, maybe we can say this is
a bug. But worker_spi is a just example of the code, I'm not thinking to
do back-patch for now. Thought?
>
> >
> > BTW, I tried to find the past discussion about why die() was not used,
> > but I could not yet.
> >
>
> I think the commit 2505ce0 that altered the comment has something:
Thanks for the info! Will check that.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 12:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-23 04:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Craig Ringer <[email protected]>
2020-11-17 12:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-18 12:22 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-20 10:33 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-25 09:59 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
@ 2020-11-25 14:38 ` Bharath Rupireddy <[email protected]>
2020-11-26 14:07 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-27 03:13 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
0 siblings, 2 replies; 38+ messages in thread
From: Bharath Rupireddy @ 2020-11-25 14:38 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: Craig Ringer <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>
On Wed, Nov 25, 2020 at 3:29 PM Fujii Masao <[email protected]>
wrote:
>
> On 2020/11/20 19:33, Bharath Rupireddy wrote:
> > On Wed, Nov 18, 2020 at 5:52 PM Fujii Masao <[email protected]
<mailto:[email protected]>> wrote:
> > >
> > > > handle_sigterm() and die() are similar except that die() has extra
> > > > handling(below) for exiting immediately when waiting for
input/command
> > > > from the client.
> > > > /*
> > > > * If we're in single user mode, we want to quit immediately
- we can't
> > > > * rely on latches as they wouldn't work when stdin/stdout is
a file.
> > > > * Rather ugly, but it's unlikely to be worthwhile to invest
much more
> > > > * effort just for the benefit of single user mode.
> > > > */
> > > > if (DoingCommandRead && whereToSendOutput != DestRemote)
> > > > ProcessInterrupts();
> > > >
> > > > Having this extra handling is correct for normal backends as they
can
> > > > connect directly to clients for reading input commands, the above
if
> > > > condition may become true and ProcessInterrupts() may be called to
> > > > handle the SIGTERM immediately.
> > > >
> > > > Note that DoingCommandRead can never be true in bg workers as it's
> > > > being set to true only in normal backend PostgresMain(). If we use
> > > > die()(instead of custom SIGTERM handlers such as handle_sigterm())
in
> > > > a bg worker/non-backend process, there are no chances that the
above
> > > > part of the code gets hit and the interrupts are processed
> > > > immediately. And also here are the bg worker process that use die()
> > > > instead of their own custom handlers: parallel workers, autoprewarm
> > > > worker, autovacuum worker, logical replication launcher and apply
> > > > worker, wal sender process
> > > >
> > > > I think we can also use die() instead of handle_sigterm() in
> > > > test_shm_mq/worker.c.
> > > >
> > > > I attached a patch for this change.
> > >
> > > Thanks for the patch! It looks good to me.
> > >
> >
> > Thanks.
>
> When I read the patch again, I found that, with the patch, the shutdown
> of worker_spi causes to report the following FATAL message.
>
> FATAL: terminating connection due to administrator command
>
> Isn't this message confusing because it's not a connection? If so,
> we need to update ProcessInterrupts() so that the proper message is
> reported like other bgworkers do.
>
This is also true for all the bgworker that use the die() handler. How
about doing it the way bgworker_die() does in ProcessInterrupts()? This
would give meaningful information. Thoughts? If okay, I can make a separate
patch.
else if (RecoveryConflictPending)
{
/* Currently there is only one non-retryable recovery conflict
*/
Assert(RecoveryConflictReason ==
PROCSIG_RECOVERY_CONFLICT_DATABASE);
pgstat_report_recovery_conflict(RecoveryConflictReason);
ereport(FATAL,
(errcode(ERRCODE_DATABASE_DROPPED),
errmsg("terminating connection due to conflict with
recovery"),
errdetail_recovery_conflict()));
}
* else if (IsBackgroundWorker) { ereport(FATAL,
(errcode(ERRCODE_ADMIN_SHUTDOWN),
errmsg("terminating background worker \"%s\" due to administrator
command", MyBgworkerEntry->bgw_type))); }*
else
ereport(FATAL,
(errcode(ERRCODE_ADMIN_SHUTDOWN),
errmsg("terminating connection due to administrator
command")));
>
> BTW, though this is not directly related to this topic, it's strange to me
> that the normal shutdown of bgworker causes to emit FATAL message
> and the message "background worker ... exited with exit code 1"
> at the normal shutdown. I've heard sometimes that users coplained that
> it's confusing to report that message for logical replication launcher
> at the normal shutdown. Maybe the mechanism to shutdown bgworker
> normally seems to have not been implemented well yet.
>
What do you mean by normal shutdown of bgworker? Is it that bgworker has
exited successfully with exit code 0 or for some reason with exit code
other than 0? Or is it when the postmaster is shutdown normally?
IIUC, when a bgworker exists either normally with an exit code 0 or other
than 0, then CleanupBackgroundWorker() is called in postmaster, a
message(like below) is prepared, and the LogChildExit() is called with
either DEBUG1 or LOG level and for instance the message you specified gets
printed "background worker ... exited with exit code 1". I have not seen a
FATAL message similar to "background worker ... exited with exit code 1" at
the normal shutdown.
snprintf(namebuf, MAXPGPATH, _("background worker \"%s\""),
rw->rw_worker.bgw_type);
LogChildExit(EXIT_STATUS_0(exitstatus) ? DEBUG1 : LOG, namebuf, pid,
exitstatus);
Am I missing something?
If my analysis is right, then for instance, when a logical replication
launcher is exited, it logs "background worker "logical replication
launcher" exited with exit code X" with either DEBUG1 or LOG level but not
with FATAL level.
>
> Without the patch, since worker_spi cannot handle the shutdown request
> promptly while it's executing the backend code, maybe we can say this is
> a bug. But worker_spi is a just example of the code, I'm not thinking to
> do back-patch for now. Thought?
>
+1 to not backpatch it.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 12:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-23 04:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Craig Ringer <[email protected]>
2020-11-17 12:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-18 12:22 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-20 10:33 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-25 09:59 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-25 14:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
@ 2020-11-26 14:07 ` Fujii Masao <[email protected]>
2020-11-26 15:15 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
1 sibling, 1 reply; 38+ messages in thread
From: Fujii Masao @ 2020-11-26 14:07 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: Craig Ringer <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>
On 2020/11/25 23:38, Bharath Rupireddy wrote:
> On Wed, Nov 25, 2020 at 3:29 PM Fujii Masao <[email protected] <mailto:[email protected]>> wrote:
> >
> > On 2020/11/20 19:33, Bharath Rupireddy wrote:
> > > On Wed, Nov 18, 2020 at 5:52 PM Fujii Masao <[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>> wrote:
> > > >
> > > > > handle_sigterm() and die() are similar except that die() has extra
> > > > > handling(below) for exiting immediately when waiting for input/command
> > > > > from the client.
> > > > > /*
> > > > > * If we're in single user mode, we want to quit immediately - we can't
> > > > > * rely on latches as they wouldn't work when stdin/stdout is a file.
> > > > > * Rather ugly, but it's unlikely to be worthwhile to invest much more
> > > > > * effort just for the benefit of single user mode.
> > > > > */
> > > > > if (DoingCommandRead && whereToSendOutput != DestRemote)
> > > > > ProcessInterrupts();
> > > > >
> > > > > Having this extra handling is correct for normal backends as they can
> > > > > connect directly to clients for reading input commands, the above if
> > > > > condition may become true and ProcessInterrupts() may be called to
> > > > > handle the SIGTERM immediately.
> > > > >
> > > > > Note that DoingCommandRead can never be true in bg workers as it's
> > > > > being set to true only in normal backend PostgresMain(). If we use
> > > > > die()(instead of custom SIGTERM handlers such as handle_sigterm()) in
> > > > > a bg worker/non-backend process, there are no chances that the above
> > > > > part of the code gets hit and the interrupts are processed
> > > > > immediately. And also here are the bg worker process that use die()
> > > > > instead of their own custom handlers: parallel workers, autoprewarm
> > > > > worker, autovacuum worker, logical replication launcher and apply
> > > > > worker, wal sender process
> > > > >
> > > > > I think we can also use die() instead of handle_sigterm() in
> > > > > test_shm_mq/worker.c.
> > > > >
> > > > > I attached a patch for this change.
> > > >
> > > > Thanks for the patch! It looks good to me.
> > > >
> > >
> > > Thanks.
> >
> > When I read the patch again, I found that, with the patch, the shutdown
> > of worker_spi causes to report the following FATAL message.
> >
> > FATAL: terminating connection due to administrator command
> >
> > Isn't this message confusing because it's not a connection? If so,
> > we need to update ProcessInterrupts() so that the proper message is
> > reported like other bgworkers do.
> >
>
> This is also true for all the bgworker that use the die() handler. How about doing it the way bgworker_die() does in ProcessInterrupts()? This would give meaningful information. Thoughts? If okay, I can make a separate patch.
+1
Thanks!
>
> else if (RecoveryConflictPending)
> {
> /* Currently there is only one non-retryable recovery conflict */
> Assert(RecoveryConflictReason == PROCSIG_RECOVERY_CONFLICT_DATABASE);
> pgstat_report_recovery_conflict(RecoveryConflictReason);
> ereport(FATAL,
> (errcode(ERRCODE_DATABASE_DROPPED),
> errmsg("terminating connection due to conflict with recovery"),
> errdetail_recovery_conflict()));
> }
> * else if (IsBackgroundWorker)
> {
> ereport(FATAL,
> (errcode(ERRCODE_ADMIN_SHUTDOWN),
> errmsg("terminating background worker \"%s\" due to administrator command",
> MyBgworkerEntry->bgw_type)));
> }*
> else
> ereport(FATAL,
> (errcode(ERRCODE_ADMIN_SHUTDOWN),
> errmsg("terminating connection due to administrator command")));
>
> >
> > BTW, though this is not directly related to this topic, it's strange to me
> > that the normal shutdown of bgworker causes to emit FATAL message
> > and the message "background worker ... exited with exit code 1"
> > at the normal shutdown. I've heard sometimes that users coplained that
> > it's confusing to report that message for logical replication launcher
> > at the normal shutdown. Maybe the mechanism to shutdown bgworker
> > normally seems to have not been implemented well yet.
> >
>
> What do you mean by normal shutdown of bgworker? Is it that bgworker has exited successfully with exit code 0 or for some reason with exit code other than 0? Or is it when the postmaster is shutdown normally?
>
> IIUC, when a bgworker exists either normally with an exit code 0 or other than 0, then CleanupBackgroundWorker() is called in postmaster, a message(like below) is prepared, and the LogChildExit() is called with either DEBUG1 or LOG level and for instance the message you specified gets printed "background worker ... exited with exit code 1". I have not seen a FATAL message similar to "background worker ... exited with exit code 1" at the normal shutdown.
>
> snprintf(namebuf, MAXPGPATH, _("background worker \"%s\""), rw->rw_worker.bgw_type);
>
> LogChildExit(EXIT_STATUS_0(exitstatus) ? DEBUG1 : LOG, namebuf, pid, exitstatus);
>
> Am I missing something?
>
> If my analysis is right, then for instance, when a logical replication launcher is exited, it logs "background worker "logical replication launcher" exited with exit code X" with either DEBUG1 or LOG level but not with FATAL level.
Yes, it's not with FATAL level. But that message looks like that it's
reporting error message. This is why we sometimes received
the complaints (e.g., [1][2]) about that message.
[1]
https://postgr.es/m/CAKFQuwZHcZnnwoFaXX2YKNT3KhaNr_+vd95Oo=f045zfn7Tetw@mail.gmail.com
[2]
https://postgr.es/m/CAEepm=1c3hG1g3iKYwfa_PDsT49RBaBJsaot_qNhPSCXBm9rzA@mail.gmail.com
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 12:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-23 04:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Craig Ringer <[email protected]>
2020-11-17 12:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-18 12:22 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-20 10:33 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-25 09:59 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-25 14:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-26 14:07 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
@ 2020-11-26 15:15 ` Bharath Rupireddy <[email protected]>
2020-11-26 15:23 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Bharath Rupireddy @ 2020-11-26 15:15 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: Craig Ringer <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>
On Thu, Nov 26, 2020 at 7:37 PM Fujii Masao <[email protected]> wrote:
>
> > What do you mean by normal shutdown of bgworker? Is it that bgworker has exited successfully with exit code 0 or for some reason with exit code other than 0? Or is it when the postmaster is shutdown normally?
> >
> > IIUC, when a bgworker exists either normally with an exit code 0 or other than 0, then CleanupBackgroundWorker() is called in postmaster, a message(like below) is prepared, and the LogChildExit() is called with either DEBUG1 or LOG level and for instance the message you specified gets printed "background worker ... exited with exit code 1". I have not seen a FATAL message similar to "background worker ... exited with exit code 1" at the normal shutdown.
> >
> > snprintf(namebuf, MAXPGPATH, _("background worker \"%s\""), rw->rw_worker.bgw_type);
> >
> > LogChildExit(EXIT_STATUS_0(exitstatus) ? DEBUG1 : LOG, namebuf, pid, exitstatus);
> >
> > Am I missing something?
> >
> > If my analysis is right, then for instance, when a logical replication launcher is exited, it logs "background worker "logical replication launcher" exited with exit code X" with either DEBUG1 or LOG level but not with FATAL level.
>
> Yes, it's not with FATAL level. But that message looks like that it's
> reporting error message. This is why we sometimes received
> the complaints (e.g., [1][2]) about that message.
>
Oh. Should we do something about it now? Any thoughts? I have not read
fully through the threads specified, I will go through them later.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 12:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-23 04:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Craig Ringer <[email protected]>
2020-11-17 12:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-18 12:22 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-20 10:33 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-25 09:59 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-25 14:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-26 14:07 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-26 15:15 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
@ 2020-11-26 15:23 ` Fujii Masao <[email protected]>
0 siblings, 0 replies; 38+ messages in thread
From: Fujii Masao @ 2020-11-26 15:23 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: Craig Ringer <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>
On 2020/11/27 0:15, Bharath Rupireddy wrote:
> On Thu, Nov 26, 2020 at 7:37 PM Fujii Masao <[email protected]> wrote:
>>
>>> What do you mean by normal shutdown of bgworker? Is it that bgworker has exited successfully with exit code 0 or for some reason with exit code other than 0? Or is it when the postmaster is shutdown normally?
>>>
>>> IIUC, when a bgworker exists either normally with an exit code 0 or other than 0, then CleanupBackgroundWorker() is called in postmaster, a message(like below) is prepared, and the LogChildExit() is called with either DEBUG1 or LOG level and for instance the message you specified gets printed "background worker ... exited with exit code 1". I have not seen a FATAL message similar to "background worker ... exited with exit code 1" at the normal shutdown.
>>>
>>> snprintf(namebuf, MAXPGPATH, _("background worker \"%s\""), rw->rw_worker.bgw_type);
>>>
>>> LogChildExit(EXIT_STATUS_0(exitstatus) ? DEBUG1 : LOG, namebuf, pid, exitstatus);
>>>
>>> Am I missing something?
>>>
>>> If my analysis is right, then for instance, when a logical replication launcher is exited, it logs "background worker "logical replication launcher" exited with exit code X" with either DEBUG1 or LOG level but not with FATAL level.
>>
>> Yes, it's not with FATAL level. But that message looks like that it's
>> reporting error message. This is why we sometimes received
>> the complaints (e.g., [1][2]) about that message.
>>
>
> Oh. Should we do something about it now?
No. This is not directly related to the issue that we are discussing
as I told upthread. Of course, it's better to work on this if we can
easily fix it. But seems not... So please ignore this my comment.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 12:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-23 04:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Craig Ringer <[email protected]>
2020-11-17 12:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-18 12:22 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-20 10:33 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-25 09:59 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-25 14:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
@ 2020-11-27 03:13 ` Bharath Rupireddy <[email protected]>
2020-11-27 06:56 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
1 sibling, 1 reply; 38+ messages in thread
From: Bharath Rupireddy @ 2020-11-27 03:13 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: Craig Ringer <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>
On Wed, Nov 25, 2020 at 8:08 PM Bharath Rupireddy <
[email protected]> wrote:
>
> > When I read the patch again, I found that, with the patch, the shutdown
> > of worker_spi causes to report the following FATAL message.
> >
> > FATAL: terminating connection due to administrator command
> >
> > Isn't this message confusing because it's not a connection? If so,
> > we need to update ProcessInterrupts() so that the proper message is
> > reported like other bgworkers do.
> >
>
> This is also true for all the bgworker that use the die() handler. How
about doing it the way bgworker_die() does in ProcessInterrupts()? This
would give meaningful information. Thoughts? If okay, I can make a separate
patch.
>
Attaching the patch that improved the message for bg workers in
ProcessInterrupts(). For instance, now it looks like *FATAL: terminating
background worker "worker_spi" due to administrator command* or *FATAL:
terminating background worker "parallel worker" due to administrator
command *and so on for other bg workers.
Please review the patch.
I'm also mentioning the 2 previous patches posted in [1]. One of the patch
is for using die() instead of handle_sigterm() in test_shm_mq/worker.c and
another is for replacing custom SIGTERM handler worker_spi_sigterm() with
die() and custom SIGHUP handler worker_spi_sighup() with standard
SignalHandlerForConfigReload()
[1]
https://www.postgresql.org/message-id/CALj2ACWWy1YcngpCUn09AsXMfOzwjfNqbVosfoRY0vhhVWhVBw%40mail.gma...
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
[application/octet-stream] v1-0001-Improve-error-message-in-ProcessInterrupts-for-bg-worker.patch (1.1K, ../../CALj2ACX4rQRHuczPnkr6Ki+PxdKR33aj9LYRFFO3t8Xgx=8eVw@mail.gmail.com/3-v1-0001-Improve-error-message-in-ProcessInterrupts-for-bg-worker.patch)
download | inline diff:
From 070d5a9a761cd1d53411b82e4e469203c2c76372 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Fri, 27 Nov 2020 07:53:33 +0530
Subject: [PATCH v1] Improve error message in ProcessInterrupts for bg workers
Shutdown of a bg worker currently produces a message
"terminating connection due to administrator command" which is
confusing, because it is not a connection. So, improve the message
for bg worker in ProcessInterrupts().
---
src/backend/tcop/postgres.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 34ed0e7558..7d7a7e0801 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3072,6 +3072,11 @@ ProcessInterrupts(void)
errmsg("terminating connection due to conflict with recovery"),
errdetail_recovery_conflict()));
}
+ else if (IsBackgroundWorker)
+ ereport(FATAL,
+ (errcode(ERRCODE_ADMIN_SHUTDOWN),
+ errmsg("terminating background worker \"%s\" due to administrator command",
+ MyBgworkerEntry->bgw_type)));
else
ereport(FATAL,
(errcode(ERRCODE_ADMIN_SHUTDOWN),
--
2.25.1
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 12:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-23 04:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Craig Ringer <[email protected]>
2020-11-17 12:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-18 12:22 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-20 10:33 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-25 09:59 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-25 14:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-27 03:13 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
@ 2020-11-27 06:56 ` Fujii Masao <[email protected]>
2020-11-27 07:47 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Fujii Masao @ 2020-11-27 06:56 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: Craig Ringer <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>
On 2020/11/27 12:13, Bharath Rupireddy wrote:
> On Wed, Nov 25, 2020 at 8:08 PM Bharath Rupireddy <[email protected] <mailto:[email protected]>> wrote:
> >
> > > When I read the patch again, I found that, with the patch, the shutdown
> > > of worker_spi causes to report the following FATAL message.
> > >
> > > FATAL: terminating connection due to administrator command
> > >
> > > Isn't this message confusing because it's not a connection? If so,
> > > we need to update ProcessInterrupts() so that the proper message is
> > > reported like other bgworkers do.
> > >
> >
> > This is also true for all the bgworker that use the die() handler. How about doing it the way bgworker_die() does in ProcessInterrupts()? This would give meaningful information. Thoughts? If okay, I can make a separate patch.
> >
>
> Attaching the patch that improved the message for bg workers in ProcessInterrupts(). For instance, now it looks like *FATAL: terminating background worker "worker_spi" due to administrator command* or *FATAL: terminating background worker "parallel worker" due to administrator command *and so on for other bg workers.*
> *
>
> Please review the patch.
Thanks for the patch! It looks good to me.
>
> I'm also mentioning the 2 previous patches posted in [1]. One of the patch is for using die() instead of handle_sigterm() in test_shm_mq/worker.c and another is for replacing custom SIGTERM handler worker_spi_sigterm() with die() and custom SIGHUP handler worker_spi_sighup() with standard SignalHandlerForConfigReload()
Yeah, I pushed them. Thanks!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 12:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-23 04:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Craig Ringer <[email protected]>
2020-11-17 12:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-18 12:22 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-20 10:33 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-25 09:59 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-25 14:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-27 03:13 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-27 06:56 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
@ 2020-11-27 07:47 ` Bharath Rupireddy <[email protected]>
2020-11-30 02:12 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Bharath Rupireddy @ 2020-11-27 07:47 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: Craig Ringer <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>
On Fri, Nov 27, 2020 at 12:26 PM Fujii Masao
<[email protected]> wrote:
>
> > > > When I read the patch again, I found that, with the patch, the shutdown
> > > > of worker_spi causes to report the following FATAL message.
> > > >
> > > > FATAL: terminating connection due to administrator command
> > > >
> > > > Isn't this message confusing because it's not a connection? If so,
> > > > we need to update ProcessInterrupts() so that the proper message is
> > > > reported like other bgworkers do.
> > > >
> > >
> > > This is also true for all the bgworker that use the die() handler. How about doing it the way bgworker_die() does in ProcessInterrupts()? This would give meaningful information. Thoughts? If okay, I can make a separate patch.
> > >
> >
> > Attaching the patch that improved the message for bg workers in ProcessInterrupts(). For instance, now it looks like *FATAL: terminating background worker "worker_spi" due to administrator command* or *FATAL: terminating background worker "parallel worker" due to administrator command *and so on for other bg workers.*
> > *
> >
> > Please review the patch.
>
> Thanks for the patch! It looks good to me.
>
Thanks!
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 12:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-23 04:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Craig Ringer <[email protected]>
2020-11-17 12:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-18 12:22 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-20 10:33 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-25 09:59 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-25 14:38 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-27 03:13 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-27 06:56 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-27 07:47 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
@ 2020-11-30 02:12 ` Fujii Masao <[email protected]>
0 siblings, 0 replies; 38+ messages in thread
From: Fujii Masao @ 2020-11-30 02:12 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: Craig Ringer <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>
On 2020/11/27 16:47, Bharath Rupireddy wrote:
> On Fri, Nov 27, 2020 at 12:26 PM Fujii Masao
> <[email protected]> wrote:
>>
>>> > > When I read the patch again, I found that, with the patch, the shutdown
>>> > > of worker_spi causes to report the following FATAL message.
>>> > >
>>> > > FATAL: terminating connection due to administrator command
>>> > >
>>> > > Isn't this message confusing because it's not a connection? If so,
>>> > > we need to update ProcessInterrupts() so that the proper message is
>>> > > reported like other bgworkers do.
>>> > >
>>> >
>>> > This is also true for all the bgworker that use the die() handler. How about doing it the way bgworker_die() does in ProcessInterrupts()? This would give meaningful information. Thoughts? If okay, I can make a separate patch.
>>> >
>>>
>>> Attaching the patch that improved the message for bg workers in ProcessInterrupts(). For instance, now it looks like *FATAL: terminating background worker "worker_spi" due to administrator command* or *FATAL: terminating background worker "parallel worker" due to administrator command *and so on for other bg workers.*
>>> *
>>>
>>> Please review the patch.
>>
>> Thanks for the patch! It looks good to me.
>>
>
> Thanks!
Pushed. Thanks!
Since all the patches that proposed in this thread were committed,
I marked the CF entry as committed.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
@ 2020-10-29 06:21 ` Fujii Masao <[email protected]>
2020-11-04 09:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
1 sibling, 1 reply; 38+ messages in thread
From: Fujii Masao @ 2020-10-29 06:21 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: pgsql-hackers
On 2020/10/07 11:30, Bharath Rupireddy wrote:
> On Tue, Oct 6, 2020 at 11:41 AM Bharath Rupireddy
> <[email protected]> wrote:
>>
>> On Tue, Oct 6, 2020 at 11:20 AM Fujii Masao <[email protected]> wrote:
>>>
>>> +1 Or it's also ok to make each patch separately.
>>> Anyway, thanks!
>>>
>>
>> Thanks. +1 to have separate patches. I will post two separate patches
>> for autoprewarm and WalRcvShutdownHandler for further review. The
>> other two patches can be for startup.c and syslogger.c.
>>
>
> I'm attaching all the 4 patches here together.
>
> 1. v1-use-standard-SIGHUP-and-SIGTERM-handlers-in-autoprewarm-process.patch
> -- This is the patch initially sent in this mail by me, I just
> renamed it.
> 2. v1-use-MyLatch-and-standard-SIGHUP-handler-in-startup-process.patch
> -- This is the patch proposed by Fuji Masao, I just renamed it.
> 3. v1-use-standard-SIGHUP-hanlder-in-syslogger-process.patch
> 4. v1-use-standard-SIGTERM-handler-in-walreceiver-process.patch
>
> Please consider these patches for further review.
Thanks for the patches! They look good to me. So barring any objections,
I will commit them one by one.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-29 06:21 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
@ 2020-11-04 09:06 ` Fujii Masao <[email protected]>
2020-11-04 15:46 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-12-15 14:10 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
0 siblings, 2 replies; 38+ messages in thread
From: Fujii Masao @ 2020-11-04 09:06 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: pgsql-hackers
On 2020/10/29 15:21, Fujii Masao wrote:
>
>
> On 2020/10/07 11:30, Bharath Rupireddy wrote:
>> On Tue, Oct 6, 2020 at 11:41 AM Bharath Rupireddy
>> <[email protected]> wrote:
>>>
>>> On Tue, Oct 6, 2020 at 11:20 AM Fujii Masao <[email protected]> wrote:
>>>>
>>>> +1 Or it's also ok to make each patch separately.
>>>> Anyway, thanks!
>>>>
>>>
>>> Thanks. +1 to have separate patches. I will post two separate patches
>>> for autoprewarm and WalRcvShutdownHandler for further review. The
>>> other two patches can be for startup.c and syslogger.c.
>>>
>>
>> I'm attaching all the 4 patches here together.
>>
>> 1. v1-use-standard-SIGHUP-and-SIGTERM-handlers-in-autoprewarm-process.patch
>> -- This is the patch initially sent in this mail by me, I just
>> renamed it.
>> 2. v1-use-MyLatch-and-standard-SIGHUP-handler-in-startup-process.patch
>> -- This is the patch proposed by Fuji Masao, I just renamed it.
>> 3. v1-use-standard-SIGHUP-hanlder-in-syslogger-process.patch
>> 4. v1-use-standard-SIGTERM-handler-in-walreceiver-process.patch
>>
>> Please consider these patches for further review.
>
> Thanks for the patches! They look good to me. So barring any objections,
> I will commit them one by one.
I pushed the following two patches.
- v1-use-standard-SIGHUP-hanlder-in-syslogger-process.patch
- v1-use-MyLatch-and-standard-SIGHUP-handler-in-startup-process.patch
Regarding other two patches, I think that it's better to use MyLatch
rather than MyProc->procLatch or walrcv->latch in WaitLatch() and
ResetLatch(), like other code does. Attached are the updated versions
of the patches. Thought?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index c32ddc56fd..d3dec6e3ec 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -35,6 +35,7 @@
#include "miscadmin.h"
#include "pgstat.h"
#include "postmaster/bgworker.h"
+#include "postmaster/interrupt.h"
#include "storage/buf_internals.h"
#include "storage/dsm.h"
#include "storage/ipc.h"
@@ -94,12 +95,6 @@ static void apw_start_database_worker(void);
static bool apw_init_shmem(void);
static void apw_detach_shmem(int code, Datum arg);
static int apw_compare_blockinfo(const void *p, const void *q);
-static void apw_sigterm_handler(SIGNAL_ARGS);
-static void apw_sighup_handler(SIGNAL_ARGS);
-
-/* Flags set by signal handlers */
-static volatile sig_atomic_t got_sigterm = false;
-static volatile sig_atomic_t got_sighup = false;
/* Pointer to shared-memory state. */
static AutoPrewarmSharedState *apw_state = NULL;
@@ -161,8 +156,8 @@ autoprewarm_main(Datum main_arg)
TimestampTz last_dump_time = 0;
/* Establish signal handlers; once that's done, unblock signals. */
- pqsignal(SIGTERM, apw_sigterm_handler);
- pqsignal(SIGHUP, apw_sighup_handler);
+ pqsignal(SIGTERM, SignalHandlerForShutdownRequest);
+ pqsignal(SIGHUP, SignalHandlerForConfigReload);
pqsignal(SIGUSR1, procsignal_sigusr1_handler);
BackgroundWorkerUnblockSignals();
@@ -206,19 +201,19 @@ autoprewarm_main(Datum main_arg)
}
/* Periodically dump buffers until terminated. */
- while (!got_sigterm)
+ while (!ShutdownRequestPending)
{
/* In case of a SIGHUP, just reload the configuration. */
- if (got_sighup)
+ if (ConfigReloadPending)
{
- got_sighup = false;
+ ConfigReloadPending = false;
ProcessConfigFile(PGC_SIGHUP);
}
if (autoprewarm_interval <= 0)
{
/* We're only dumping at shutdown, so just wait forever. */
- (void) WaitLatch(&MyProc->procLatch,
+ (void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_EXIT_ON_PM_DEATH,
-1L,
PG_WAIT_EXTENSION);
@@ -247,14 +242,14 @@ autoprewarm_main(Datum main_arg)
}
/* Sleep until the next dump time. */
- (void) WaitLatch(&MyProc->procLatch,
+ (void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
delay_in_ms,
PG_WAIT_EXTENSION);
}
/* Reset the latch, loop. */
- ResetLatch(&MyProc->procLatch);
+ ResetLatch(MyLatch);
}
/*
@@ -895,35 +890,3 @@ apw_compare_blockinfo(const void *p, const void *q)
return 0;
}
-
-/*
- * Signal handler for SIGTERM
- */
-static void
-apw_sigterm_handler(SIGNAL_ARGS)
-{
- int save_errno = errno;
-
- got_sigterm = true;
-
- if (MyProc)
- SetLatch(&MyProc->procLatch);
-
- errno = save_errno;
-}
-
-/*
- * Signal handler for SIGHUP
- */
-static void
-apw_sighup_handler(SIGNAL_ARGS)
-{
- int save_errno = errno;
-
- got_sighup = true;
-
- if (MyProc)
- SetLatch(&MyProc->procLatch);
-
- errno = save_errno;
-}
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index bb1d44ccb7..ff0dc7925c 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -109,7 +109,6 @@ static XLogSegNo recvSegNo = 0;
* main loop.
*/
static volatile sig_atomic_t got_SIGHUP = false;
-static volatile sig_atomic_t got_SIGTERM = false;
/*
* LogstreamResult indicates the byte positions that we have already
@@ -137,8 +136,6 @@ static void ProcessWalSndrMessage(XLogRecPtr walEnd, TimestampTz sendTime);
/* Signal handlers */
static void WalRcvSigHupHandler(SIGNAL_ARGS);
-static void WalRcvShutdownHandler(SIGNAL_ARGS);
-
/*
* Process any interrupts the walreceiver process may have received.
@@ -164,7 +161,7 @@ ProcessWalRcvInterrupts(void)
*/
CHECK_FOR_INTERRUPTS();
- if (got_SIGTERM)
+ if (ShutdownRequestPending)
{
ereport(FATAL,
(errcode(ERRCODE_ADMIN_SHUTDOWN),
@@ -269,7 +266,7 @@ WalReceiverMain(void)
/* Properly accept or ignore signals the postmaster might send us */
pqsignal(SIGHUP, WalRcvSigHupHandler); /* set flag to read config file */
pqsignal(SIGINT, SIG_IGN);
- pqsignal(SIGTERM, WalRcvShutdownHandler); /* request shutdown */
+ pqsignal(SIGTERM, SignalHandlerForShutdownRequest); /* request shutdown */
/* SIGQUIT handler was already set up by InitPostmasterChild */
pqsignal(SIGALRM, SIG_IGN);
pqsignal(SIGPIPE, SIG_IGN);
@@ -510,7 +507,7 @@ WalReceiverMain(void)
* avoiding some system calls.
*/
Assert(wait_fd != PGINVALID_SOCKET);
- rc = WaitLatchOrSocket(walrcv->latch,
+ rc = WaitLatchOrSocket(MyLatch,
WL_EXIT_ON_PM_DEATH | WL_SOCKET_READABLE |
WL_TIMEOUT | WL_LATCH_SET,
wait_fd,
@@ -518,7 +515,7 @@ WalReceiverMain(void)
WAIT_EVENT_WAL_RECEIVER_MAIN);
if (rc & WL_LATCH_SET)
{
- ResetLatch(walrcv->latch);
+ ResetLatch(MyLatch);
ProcessWalRcvInterrupts();
if (walrcv->force_reply)
@@ -669,7 +666,7 @@ WalRcvWaitForStartPosition(XLogRecPtr *startpoint, TimeLineID *startpointTLI)
WakeupRecovery();
for (;;)
{
- ResetLatch(walrcv->latch);
+ ResetLatch(MyLatch);
ProcessWalRcvInterrupts();
@@ -701,7 +698,7 @@ WalRcvWaitForStartPosition(XLogRecPtr *startpoint, TimeLineID *startpointTLI)
}
SpinLockRelease(&walrcv->mutex);
- (void) WaitLatch(walrcv->latch, WL_LATCH_SET | WL_EXIT_ON_PM_DEATH, 0,
+ (void) WaitLatch(MyLatch, WL_LATCH_SET | WL_EXIT_ON_PM_DEATH, 0,
WAIT_EVENT_WAL_RECEIVER_WAIT_START);
}
@@ -813,21 +810,6 @@ WalRcvSigHupHandler(SIGNAL_ARGS)
got_SIGHUP = true;
}
-
-/* SIGTERM: set flag for ProcessWalRcvInterrupts */
-static void
-WalRcvShutdownHandler(SIGNAL_ARGS)
-{
- int save_errno = errno;
-
- got_SIGTERM = true;
-
- if (WalRcv->latch)
- SetLatch(WalRcv->latch);
-
- errno = save_errno;
-}
-
/*
* Accept the message from XLOG stream, and process it.
*/
Attachments:
[text/plain] v2-use-standard-SIGHUP-and-SIGTERM-handlers-in-autoprewarm-process.patch (2.9K, ../../[email protected]/2-v2-use-standard-SIGHUP-and-SIGTERM-handlers-in-autoprewarm-process.patch)
download | inline diff:
diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index c32ddc56fd..d3dec6e3ec 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -35,6 +35,7 @@
#include "miscadmin.h"
#include "pgstat.h"
#include "postmaster/bgworker.h"
+#include "postmaster/interrupt.h"
#include "storage/buf_internals.h"
#include "storage/dsm.h"
#include "storage/ipc.h"
@@ -94,12 +95,6 @@ static void apw_start_database_worker(void);
static bool apw_init_shmem(void);
static void apw_detach_shmem(int code, Datum arg);
static int apw_compare_blockinfo(const void *p, const void *q);
-static void apw_sigterm_handler(SIGNAL_ARGS);
-static void apw_sighup_handler(SIGNAL_ARGS);
-
-/* Flags set by signal handlers */
-static volatile sig_atomic_t got_sigterm = false;
-static volatile sig_atomic_t got_sighup = false;
/* Pointer to shared-memory state. */
static AutoPrewarmSharedState *apw_state = NULL;
@@ -161,8 +156,8 @@ autoprewarm_main(Datum main_arg)
TimestampTz last_dump_time = 0;
/* Establish signal handlers; once that's done, unblock signals. */
- pqsignal(SIGTERM, apw_sigterm_handler);
- pqsignal(SIGHUP, apw_sighup_handler);
+ pqsignal(SIGTERM, SignalHandlerForShutdownRequest);
+ pqsignal(SIGHUP, SignalHandlerForConfigReload);
pqsignal(SIGUSR1, procsignal_sigusr1_handler);
BackgroundWorkerUnblockSignals();
@@ -206,19 +201,19 @@ autoprewarm_main(Datum main_arg)
}
/* Periodically dump buffers until terminated. */
- while (!got_sigterm)
+ while (!ShutdownRequestPending)
{
/* In case of a SIGHUP, just reload the configuration. */
- if (got_sighup)
+ if (ConfigReloadPending)
{
- got_sighup = false;
+ ConfigReloadPending = false;
ProcessConfigFile(PGC_SIGHUP);
}
if (autoprewarm_interval <= 0)
{
/* We're only dumping at shutdown, so just wait forever. */
- (void) WaitLatch(&MyProc->procLatch,
+ (void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_EXIT_ON_PM_DEATH,
-1L,
PG_WAIT_EXTENSION);
@@ -247,14 +242,14 @@ autoprewarm_main(Datum main_arg)
}
/* Sleep until the next dump time. */
- (void) WaitLatch(&MyProc->procLatch,
+ (void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
delay_in_ms,
PG_WAIT_EXTENSION);
}
/* Reset the latch, loop. */
- ResetLatch(&MyProc->procLatch);
+ ResetLatch(MyLatch);
}
/*
@@ -895,35 +890,3 @@ apw_compare_blockinfo(const void *p, const void *q)
return 0;
}
-
-/*
- * Signal handler for SIGTERM
- */
-static void
-apw_sigterm_handler(SIGNAL_ARGS)
-{
- int save_errno = errno;
-
- got_sigterm = true;
-
- if (MyProc)
- SetLatch(&MyProc->procLatch);
-
- errno = save_errno;
-}
-
-/*
- * Signal handler for SIGHUP
- */
-static void
-apw_sighup_handler(SIGNAL_ARGS)
-{
- int save_errno = errno;
-
- got_sighup = true;
-
- if (MyProc)
- SetLatch(&MyProc->procLatch);
-
- errno = save_errno;
-}
[text/plain] v2-use-standard-SIGTERM-handler-in-walreceiver-process.patch (2.9K, ../../[email protected]/3-v2-use-standard-SIGTERM-handler-in-walreceiver-process.patch)
download | inline diff:
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index bb1d44ccb7..ff0dc7925c 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -109,7 +109,6 @@ static XLogSegNo recvSegNo = 0;
* main loop.
*/
static volatile sig_atomic_t got_SIGHUP = false;
-static volatile sig_atomic_t got_SIGTERM = false;
/*
* LogstreamResult indicates the byte positions that we have already
@@ -137,8 +136,6 @@ static void ProcessWalSndrMessage(XLogRecPtr walEnd, TimestampTz sendTime);
/* Signal handlers */
static void WalRcvSigHupHandler(SIGNAL_ARGS);
-static void WalRcvShutdownHandler(SIGNAL_ARGS);
-
/*
* Process any interrupts the walreceiver process may have received.
@@ -164,7 +161,7 @@ ProcessWalRcvInterrupts(void)
*/
CHECK_FOR_INTERRUPTS();
- if (got_SIGTERM)
+ if (ShutdownRequestPending)
{
ereport(FATAL,
(errcode(ERRCODE_ADMIN_SHUTDOWN),
@@ -269,7 +266,7 @@ WalReceiverMain(void)
/* Properly accept or ignore signals the postmaster might send us */
pqsignal(SIGHUP, WalRcvSigHupHandler); /* set flag to read config file */
pqsignal(SIGINT, SIG_IGN);
- pqsignal(SIGTERM, WalRcvShutdownHandler); /* request shutdown */
+ pqsignal(SIGTERM, SignalHandlerForShutdownRequest); /* request shutdown */
/* SIGQUIT handler was already set up by InitPostmasterChild */
pqsignal(SIGALRM, SIG_IGN);
pqsignal(SIGPIPE, SIG_IGN);
@@ -510,7 +507,7 @@ WalReceiverMain(void)
* avoiding some system calls.
*/
Assert(wait_fd != PGINVALID_SOCKET);
- rc = WaitLatchOrSocket(walrcv->latch,
+ rc = WaitLatchOrSocket(MyLatch,
WL_EXIT_ON_PM_DEATH | WL_SOCKET_READABLE |
WL_TIMEOUT | WL_LATCH_SET,
wait_fd,
@@ -518,7 +515,7 @@ WalReceiverMain(void)
WAIT_EVENT_WAL_RECEIVER_MAIN);
if (rc & WL_LATCH_SET)
{
- ResetLatch(walrcv->latch);
+ ResetLatch(MyLatch);
ProcessWalRcvInterrupts();
if (walrcv->force_reply)
@@ -669,7 +666,7 @@ WalRcvWaitForStartPosition(XLogRecPtr *startpoint, TimeLineID *startpointTLI)
WakeupRecovery();
for (;;)
{
- ResetLatch(walrcv->latch);
+ ResetLatch(MyLatch);
ProcessWalRcvInterrupts();
@@ -701,7 +698,7 @@ WalRcvWaitForStartPosition(XLogRecPtr *startpoint, TimeLineID *startpointTLI)
}
SpinLockRelease(&walrcv->mutex);
- (void) WaitLatch(walrcv->latch, WL_LATCH_SET | WL_EXIT_ON_PM_DEATH, 0,
+ (void) WaitLatch(MyLatch, WL_LATCH_SET | WL_EXIT_ON_PM_DEATH, 0,
WAIT_EVENT_WAL_RECEIVER_WAIT_START);
}
@@ -813,21 +810,6 @@ WalRcvSigHupHandler(SIGNAL_ARGS)
got_SIGHUP = true;
}
-
-/* SIGTERM: set flag for ProcessWalRcvInterrupts */
-static void
-WalRcvShutdownHandler(SIGNAL_ARGS)
-{
- int save_errno = errno;
-
- got_SIGTERM = true;
-
- if (WalRcv->latch)
- SetLatch(WalRcv->latch);
-
- errno = save_errno;
-}
-
/*
* Accept the message from XLOG stream, and process it.
*/
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-29 06:21 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 09:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
@ 2020-11-04 15:46 ` Bharath Rupireddy <[email protected]>
2020-11-05 03:12 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Kyotaro Horiguchi <[email protected]>
1 sibling, 1 reply; 38+ messages in thread
From: Bharath Rupireddy @ 2020-11-04 15:46 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: pgsql-hackers
On Wed, Nov 4, 2020 at 2:36 PM Fujii Masao <[email protected]> wrote:
>
> Regarding other two patches, I think that it's better to use MyLatch
> rather than MyProc->procLatch or walrcv->latch in WaitLatch() and
> ResetLatch(), like other code does. Attached are the updated versions
> of the patches. Thought?
>
+1 for replacing MyProc->procLatch with MyLatch in the autoprewarm
module, and the patch looks good to me.
I'm not quite sure to replace all the places in the walreceiver
process, for instance in WalRcvForceReply() we are using spinlock to
acquire the latch pointer and . Others may have better thoughts on
this.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-29 06:21 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 09:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 15:46 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
@ 2020-11-05 03:12 ` Kyotaro Horiguchi <[email protected]>
2020-11-06 17:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Kyotaro Horiguchi @ 2020-11-05 03:12 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; pgsql-hackers
At Wed, 4 Nov 2020 21:16:29 +0530, Bharath Rupireddy <[email protected]> wrote in
> On Wed, Nov 4, 2020 at 2:36 PM Fujii Masao <[email protected]> wrote:
> >
> > Regarding other two patches, I think that it's better to use MyLatch
> > rather than MyProc->procLatch or walrcv->latch in WaitLatch() and
> > ResetLatch(), like other code does. Attached are the updated versions
> > of the patches. Thought?
> >
>
> +1 for replacing MyProc->procLatch with MyLatch in the autoprewarm
> module, and the patch looks good to me.
Looks good to me, too.
> I'm not quite sure to replace all the places in the walreceiver
> process, for instance in WalRcvForceReply() we are using spinlock to
> acquire the latch pointer and . Others may have better thoughts on
> this.
The SIGTERM part looks good. The only difference between
WalRcvSigHupHandler and SignalHandlerForConfigReload is whether latch
is set or not. I don't think it's a problem that config-reload causes
an extra wakeup. Couldn't we do the same thing for SIGHUP?
We might even be able to reload config file in
ProcessWalRcvInterrupts().
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-29 06:21 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 09:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 15:46 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-05 03:12 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Kyotaro Horiguchi <[email protected]>
@ 2020-11-06 17:30 ` Fujii Masao <[email protected]>
2020-11-07 14:01 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Fujii Masao @ 2020-11-06 17:30 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; [email protected]; +Cc: pgsql-hackers
On 2020/11/05 12:12, Kyotaro Horiguchi wrote:
> At Wed, 4 Nov 2020 21:16:29 +0530, Bharath Rupireddy <[email protected]> wrote in
>> On Wed, Nov 4, 2020 at 2:36 PM Fujii Masao <[email protected]> wrote:
>>>
>>> Regarding other two patches, I think that it's better to use MyLatch
>>> rather than MyProc->procLatch or walrcv->latch in WaitLatch() and
>>> ResetLatch(), like other code does. Attached are the updated versions
>>> of the patches. Thought?
>>>
>>
>> +1 for replacing MyProc->procLatch with MyLatch in the autoprewarm
>> module, and the patch looks good to me.
>
> Looks good to me, too.
Thanks for the review! I pushed the patch.
>
>> I'm not quite sure to replace all the places in the walreceiver
>> process, for instance in WalRcvForceReply() we are using spinlock to
>> acquire the latch pointer and . Others may have better thoughts on
>> this.
>
> The SIGTERM part looks good. The only difference between
> WalRcvSigHupHandler and SignalHandlerForConfigReload is whether latch
> is set or not. I don't think it's a problem that config-reload causes
> an extra wakeup. Couldn't we do the same thing for SIGHUP?
I agree that we can use even standard SIGHUP handler in walreceiver.
I'm not sure why SetLatch() was not called in walreceiver's SIGHUP
handler so far.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-29 06:21 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 09:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 15:46 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-05 03:12 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Kyotaro Horiguchi <[email protected]>
2020-11-06 17:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
@ 2020-11-07 14:01 ` Bharath Rupireddy <[email protected]>
2020-11-10 07:17 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Bharath Rupireddy @ 2020-11-07 14:01 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; pgsql-hackers
On Fri, Nov 6, 2020 at 11:00 PM Fujii Masao <[email protected]> wrote:
>
> >> I'm not quite sure to replace all the places in the walreceiver
> >> process, for instance in WalRcvForceReply() we are using spinlock to
> >> acquire the latch pointer and . Others may have better thoughts on
> >> this.
> >
> > The SIGTERM part looks good. The only difference between
> > WalRcvSigHupHandler and SignalHandlerForConfigReload is whether latch
> > is set or not. I don't think it's a problem that config-reload causes
> > an extra wakeup. Couldn't we do the same thing for SIGHUP?
>
> I agree that we can use even standard SIGHUP handler in walreceiver.
> I'm not sure why SetLatch() was not called in walreceiver's SIGHUP
> handler so far.
>
The main reason for having SetLatch() in
SignalHandlerForConfigReload() is to wake up the calling process if
waiting in WaitLatchOrSocket() or WaitLatch() and reload the new
config file and use the reloaded config variables. Maybe we should
give a thought on the scenarios in which the walreceiver process
waits, and what happens in case the latch is set when SIGHUP is
received.
And also, I think it's worth having a look at the commit 40f908bdcdc7
that introduced WalRcvSigHupHandler() and 597a87ccc that replaced
custom latch with procLatch.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-29 06:21 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 09:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 15:46 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-05 03:12 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Kyotaro Horiguchi <[email protected]>
2020-11-06 17:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-07 14:01 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
@ 2020-11-10 07:17 ` Kyotaro Horiguchi <[email protected]>
2020-11-10 09:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Kyotaro Horiguchi @ 2020-11-10 07:17 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; pgsql-hackers
At Sat, 7 Nov 2020 19:31:21 +0530, Bharath Rupireddy <[email protected]> wrote in
> The main reason for having SetLatch() in
> SignalHandlerForConfigReload() is to wake up the calling process if
> waiting in WaitLatchOrSocket() or WaitLatch() and reload the new
> config file and use the reloaded config variables. Maybe we should
> give a thought on the scenarios in which the walreceiver process
> waits, and what happens in case the latch is set when SIGHUP is
> received.
The difference is whether the config file is processed at the next
wakeup (by force-reply-request or SIGTERM) of walreceiver or
immediately. If server-reload happened frequently, say, several times
per second(?), we should consider to reduce the useless reloading, but
actually that's not the case.
> And also, I think it's worth having a look at the commit 40f908bdcdc7
> that introduced WalRcvSigHupHandler() and 597a87ccc that replaced
> custom latch with procLatch.
At the time of the first patch, PostgreSQL processes used arbitrarily
implemented SIGHUP handlers for their own so it is natural that
walreceiver used its own one.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-29 06:21 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 09:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 15:46 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-05 03:12 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Kyotaro Horiguchi <[email protected]>
2020-11-06 17:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-07 14:01 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-10 07:17 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Kyotaro Horiguchi <[email protected]>
@ 2020-11-10 09:34 ` Fujii Masao <[email protected]>
2020-11-10 12:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Fujii Masao @ 2020-11-10 09:34 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; [email protected]; +Cc: pgsql-hackers
On 2020/11/10 16:17, Kyotaro Horiguchi wrote:
> At Sat, 7 Nov 2020 19:31:21 +0530, Bharath Rupireddy <[email protected]> wrote in
>> The main reason for having SetLatch() in
>> SignalHandlerForConfigReload() is to wake up the calling process if
>> waiting in WaitLatchOrSocket() or WaitLatch() and reload the new
>> config file and use the reloaded config variables. Maybe we should
>> give a thought on the scenarios in which the walreceiver process
>> waits, and what happens in case the latch is set when SIGHUP is
>> received.
>
> The difference is whether the config file is processed at the next
> wakeup (by force-reply-request or SIGTERM) of walreceiver or
> immediately. If server-reload happened frequently, say, several times
> per second(?), we should consider to reduce the useless reloading, but
> actually that's not the case.
So, attached is the patch that makes walreceiver use both standard
SIGTERM and SIGHUP handlers. Currently I've not found any actual
issues by making walreceiver use standard SIGHUP handler, yet.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index bb1d44ccb7..babee386c4 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -104,13 +104,6 @@ static int recvFile = -1;
static TimeLineID recvFileTLI = 0;
static XLogSegNo recvSegNo = 0;
-/*
- * Flags set by interrupt handlers of walreceiver for later service in the
- * main loop.
- */
-static volatile sig_atomic_t got_SIGHUP = false;
-static volatile sig_atomic_t got_SIGTERM = false;
-
/*
* LogstreamResult indicates the byte positions that we have already
* written/fsynced.
@@ -135,11 +128,6 @@ static void XLogWalRcvSendReply(bool force, bool requestReply);
static void XLogWalRcvSendHSFeedback(bool immed);
static void ProcessWalSndrMessage(XLogRecPtr walEnd, TimestampTz sendTime);
-/* Signal handlers */
-static void WalRcvSigHupHandler(SIGNAL_ARGS);
-static void WalRcvShutdownHandler(SIGNAL_ARGS);
-
-
/*
* Process any interrupts the walreceiver process may have received.
* This should be called any time the process's latch has become set.
@@ -164,7 +152,7 @@ ProcessWalRcvInterrupts(void)
*/
CHECK_FOR_INTERRUPTS();
- if (got_SIGTERM)
+ if (ShutdownRequestPending)
{
ereport(FATAL,
(errcode(ERRCODE_ADMIN_SHUTDOWN),
@@ -267,9 +255,10 @@ WalReceiverMain(void)
on_shmem_exit(WalRcvDie, 0);
/* Properly accept or ignore signals the postmaster might send us */
- pqsignal(SIGHUP, WalRcvSigHupHandler); /* set flag to read config file */
+ pqsignal(SIGHUP, SignalHandlerForConfigReload); /* set flag to read config
+ * file */
pqsignal(SIGINT, SIG_IGN);
- pqsignal(SIGTERM, WalRcvShutdownHandler); /* request shutdown */
+ pqsignal(SIGTERM, SignalHandlerForShutdownRequest); /* request shutdown */
/* SIGQUIT handler was already set up by InitPostmasterChild */
pqsignal(SIGALRM, SIG_IGN);
pqsignal(SIGPIPE, SIG_IGN);
@@ -441,9 +430,9 @@ WalReceiverMain(void)
/* Process any requests or signals received recently */
ProcessWalRcvInterrupts();
- if (got_SIGHUP)
+ if (ConfigReloadPending)
{
- got_SIGHUP = false;
+ ConfigReloadPending = false;
ProcessConfigFile(PGC_SIGHUP);
XLogWalRcvSendHSFeedback(true);
}
@@ -510,7 +499,7 @@ WalReceiverMain(void)
* avoiding some system calls.
*/
Assert(wait_fd != PGINVALID_SOCKET);
- rc = WaitLatchOrSocket(walrcv->latch,
+ rc = WaitLatchOrSocket(MyLatch,
WL_EXIT_ON_PM_DEATH | WL_SOCKET_READABLE |
WL_TIMEOUT | WL_LATCH_SET,
wait_fd,
@@ -518,7 +507,7 @@ WalReceiverMain(void)
WAIT_EVENT_WAL_RECEIVER_MAIN);
if (rc & WL_LATCH_SET)
{
- ResetLatch(walrcv->latch);
+ ResetLatch(MyLatch);
ProcessWalRcvInterrupts();
if (walrcv->force_reply)
@@ -669,7 +658,7 @@ WalRcvWaitForStartPosition(XLogRecPtr *startpoint, TimeLineID *startpointTLI)
WakeupRecovery();
for (;;)
{
- ResetLatch(walrcv->latch);
+ ResetLatch(MyLatch);
ProcessWalRcvInterrupts();
@@ -701,7 +690,7 @@ WalRcvWaitForStartPosition(XLogRecPtr *startpoint, TimeLineID *startpointTLI)
}
SpinLockRelease(&walrcv->mutex);
- (void) WaitLatch(walrcv->latch, WL_LATCH_SET | WL_EXIT_ON_PM_DEATH, 0,
+ (void) WaitLatch(MyLatch, WL_LATCH_SET | WL_EXIT_ON_PM_DEATH, 0,
WAIT_EVENT_WAL_RECEIVER_WAIT_START);
}
@@ -806,28 +795,6 @@ WalRcvDie(int code, Datum arg)
WakeupRecovery();
}
-/* SIGHUP: set flag to re-read config file at next convenient time */
-static void
-WalRcvSigHupHandler(SIGNAL_ARGS)
-{
- got_SIGHUP = true;
-}
-
-
-/* SIGTERM: set flag for ProcessWalRcvInterrupts */
-static void
-WalRcvShutdownHandler(SIGNAL_ARGS)
-{
- int save_errno = errno;
-
- got_SIGTERM = true;
-
- if (WalRcv->latch)
- SetLatch(WalRcv->latch);
-
- errno = save_errno;
-}
-
/*
* Accept the message from XLOG stream, and process it.
*/
Attachments:
[text/plain] v3-use-standard-SIGTERM-and-SIGHUP-handlers-in-walreceiver-process.patch (3.8K, ../../[email protected]/2-v3-use-standard-SIGTERM-and-SIGHUP-handlers-in-walreceiver-process.patch)
download | inline diff:
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index bb1d44ccb7..babee386c4 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -104,13 +104,6 @@ static int recvFile = -1;
static TimeLineID recvFileTLI = 0;
static XLogSegNo recvSegNo = 0;
-/*
- * Flags set by interrupt handlers of walreceiver for later service in the
- * main loop.
- */
-static volatile sig_atomic_t got_SIGHUP = false;
-static volatile sig_atomic_t got_SIGTERM = false;
-
/*
* LogstreamResult indicates the byte positions that we have already
* written/fsynced.
@@ -135,11 +128,6 @@ static void XLogWalRcvSendReply(bool force, bool requestReply);
static void XLogWalRcvSendHSFeedback(bool immed);
static void ProcessWalSndrMessage(XLogRecPtr walEnd, TimestampTz sendTime);
-/* Signal handlers */
-static void WalRcvSigHupHandler(SIGNAL_ARGS);
-static void WalRcvShutdownHandler(SIGNAL_ARGS);
-
-
/*
* Process any interrupts the walreceiver process may have received.
* This should be called any time the process's latch has become set.
@@ -164,7 +152,7 @@ ProcessWalRcvInterrupts(void)
*/
CHECK_FOR_INTERRUPTS();
- if (got_SIGTERM)
+ if (ShutdownRequestPending)
{
ereport(FATAL,
(errcode(ERRCODE_ADMIN_SHUTDOWN),
@@ -267,9 +255,10 @@ WalReceiverMain(void)
on_shmem_exit(WalRcvDie, 0);
/* Properly accept or ignore signals the postmaster might send us */
- pqsignal(SIGHUP, WalRcvSigHupHandler); /* set flag to read config file */
+ pqsignal(SIGHUP, SignalHandlerForConfigReload); /* set flag to read config
+ * file */
pqsignal(SIGINT, SIG_IGN);
- pqsignal(SIGTERM, WalRcvShutdownHandler); /* request shutdown */
+ pqsignal(SIGTERM, SignalHandlerForShutdownRequest); /* request shutdown */
/* SIGQUIT handler was already set up by InitPostmasterChild */
pqsignal(SIGALRM, SIG_IGN);
pqsignal(SIGPIPE, SIG_IGN);
@@ -441,9 +430,9 @@ WalReceiverMain(void)
/* Process any requests or signals received recently */
ProcessWalRcvInterrupts();
- if (got_SIGHUP)
+ if (ConfigReloadPending)
{
- got_SIGHUP = false;
+ ConfigReloadPending = false;
ProcessConfigFile(PGC_SIGHUP);
XLogWalRcvSendHSFeedback(true);
}
@@ -510,7 +499,7 @@ WalReceiverMain(void)
* avoiding some system calls.
*/
Assert(wait_fd != PGINVALID_SOCKET);
- rc = WaitLatchOrSocket(walrcv->latch,
+ rc = WaitLatchOrSocket(MyLatch,
WL_EXIT_ON_PM_DEATH | WL_SOCKET_READABLE |
WL_TIMEOUT | WL_LATCH_SET,
wait_fd,
@@ -518,7 +507,7 @@ WalReceiverMain(void)
WAIT_EVENT_WAL_RECEIVER_MAIN);
if (rc & WL_LATCH_SET)
{
- ResetLatch(walrcv->latch);
+ ResetLatch(MyLatch);
ProcessWalRcvInterrupts();
if (walrcv->force_reply)
@@ -669,7 +658,7 @@ WalRcvWaitForStartPosition(XLogRecPtr *startpoint, TimeLineID *startpointTLI)
WakeupRecovery();
for (;;)
{
- ResetLatch(walrcv->latch);
+ ResetLatch(MyLatch);
ProcessWalRcvInterrupts();
@@ -701,7 +690,7 @@ WalRcvWaitForStartPosition(XLogRecPtr *startpoint, TimeLineID *startpointTLI)
}
SpinLockRelease(&walrcv->mutex);
- (void) WaitLatch(walrcv->latch, WL_LATCH_SET | WL_EXIT_ON_PM_DEATH, 0,
+ (void) WaitLatch(MyLatch, WL_LATCH_SET | WL_EXIT_ON_PM_DEATH, 0,
WAIT_EVENT_WAL_RECEIVER_WAIT_START);
}
@@ -806,28 +795,6 @@ WalRcvDie(int code, Datum arg)
WakeupRecovery();
}
-/* SIGHUP: set flag to re-read config file at next convenient time */
-static void
-WalRcvSigHupHandler(SIGNAL_ARGS)
-{
- got_SIGHUP = true;
-}
-
-
-/* SIGTERM: set flag for ProcessWalRcvInterrupts */
-static void
-WalRcvShutdownHandler(SIGNAL_ARGS)
-{
- int save_errno = errno;
-
- got_SIGTERM = true;
-
- if (WalRcv->latch)
- SetLatch(WalRcv->latch);
-
- errno = save_errno;
-}
-
/*
* Accept the message from XLOG stream, and process it.
*/
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-29 06:21 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 09:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 15:46 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-05 03:12 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Kyotaro Horiguchi <[email protected]>
2020-11-06 17:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-07 14:01 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-10 07:17 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Kyotaro Horiguchi <[email protected]>
2020-11-10 09:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
@ 2020-11-10 12:30 ` Bharath Rupireddy <[email protected]>
2020-11-10 13:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-12 04:36 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
0 siblings, 2 replies; 38+ messages in thread
From: Bharath Rupireddy @ 2020-11-10 12:30 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; pgsql-hackers
On Tue, Nov 10, 2020 at 3:04 PM Fujii Masao <[email protected]> wrote:
>
> >> The main reason for having SetLatch() in
> >> SignalHandlerForConfigReload() is to wake up the calling process if
> >> waiting in WaitLatchOrSocket() or WaitLatch() and reload the new
> >> config file and use the reloaded config variables. Maybe we should
> >> give a thought on the scenarios in which the walreceiver process
> >> waits, and what happens in case the latch is set when SIGHUP is
> >> received.
> >
> > The difference is whether the config file is processed at the next
> > wakeup (by force-reply-request or SIGTERM) of walreceiver or
> > immediately. If server-reload happened frequently, say, several times
> > per second(?), we should consider to reduce the useless reloading, but
> > actually that's not the case.
>
> So, attached is the patch that makes walreceiver use both standard
> SIGTERM and SIGHUP handlers. Currently I've not found any actual
> issues by making walreceiver use standard SIGHUP handler, yet.
>
I think it makes sense to replace WalRcv->latch with
MyProc->procLatch(as they point to the same latch) in the functions
that are called in the walreceiver process. However, we are using
walrcv->latch with spinlock, say in WalRcvForceReply() and
RequestXLogStreaming() both are called from the startup process. See
commit 45f9d08684, that made the access to the walreceiver's
latch(WalRcv->latch) by the other process(startup) spinlock protected
And looks like, in general it's a standard practice to set latch to
wake up the process if waiting in case a SIGHUP signal is received and
reload the relevant config variables.
Going by the above analysis, the v3 patch looks good to me.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-29 06:21 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 09:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 15:46 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-05 03:12 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Kyotaro Horiguchi <[email protected]>
2020-11-06 17:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-07 14:01 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-10 07:17 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Kyotaro Horiguchi <[email protected]>
2020-11-10 09:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-10 12:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
@ 2020-11-10 13:11 ` Bharath Rupireddy <[email protected]>
1 sibling, 0 replies; 38+ messages in thread
From: Bharath Rupireddy @ 2020-11-10 13:11 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; pgsql-hackers
Hi,
Attaching a patch that replaces custom signal handlers for SIGHUP and
SIGTERM in worker_spi.c.
Thoughts?
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
[application/x-patch] v1-0001-Use-standard-SIGHUP-SIGTERM-handlers-in-worker_sp.patch (2.8K, ../../CALj2ACXDEZhAFOTDcqO9cFSRvrFLyYOnPKrcA1UG4uZn9hUAVg@mail.gmail.com/2-v1-0001-Use-standard-SIGHUP-SIGTERM-handlers-in-worker_sp.patch)
download | inline diff:
From a212163b64bc3ab4b8d4493e6d53f32979e3e9bf Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Tue, 10 Nov 2020 18:27:00 +0530
Subject: [PATCH v1] Use-standard-SIGHUP-SIGTERM-handlers-in-worker_spi.c
---
src/test/modules/worker_spi/worker_spi.c | 47 +++---------------------
1 file changed, 6 insertions(+), 41 deletions(-)
diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index 258237f9bf..5598d2e850 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -25,6 +25,7 @@
/* These are always necessary for a bgworker */
#include "miscadmin.h"
#include "postmaster/bgworker.h"
+#include "postmaster/interrupt.h"
#include "storage/ipc.h"
#include "storage/latch.h"
#include "storage/lwlock.h"
@@ -48,10 +49,6 @@ PG_FUNCTION_INFO_V1(worker_spi_launch);
void _PG_init(void);
void worker_spi_main(Datum) pg_attribute_noreturn();
-/* flags set by signal handlers */
-static volatile sig_atomic_t got_sighup = false;
-static volatile sig_atomic_t got_sigterm = false;
-
/* GUC variables */
static int worker_spi_naptime = 10;
static int worker_spi_total_workers = 2;
@@ -64,38 +61,6 @@ typedef struct worktable
const char *name;
} worktable;
-/*
- * Signal handler for SIGTERM
- * Set a flag to let the main loop to terminate, and set our latch to wake
- * it up.
- */
-static void
-worker_spi_sigterm(SIGNAL_ARGS)
-{
- int save_errno = errno;
-
- got_sigterm = true;
- SetLatch(MyLatch);
-
- errno = save_errno;
-}
-
-/*
- * Signal handler for SIGHUP
- * Set a flag to tell the main loop to reread the config file, and set
- * our latch to wake it up.
- */
-static void
-worker_spi_sighup(SIGNAL_ARGS)
-{
- int save_errno = errno;
-
- got_sighup = true;
- SetLatch(MyLatch);
-
- errno = save_errno;
-}
-
/*
* Initialize workspace for a worker process: create the schema if it doesn't
* already exist.
@@ -179,8 +144,8 @@ worker_spi_main(Datum main_arg)
table->name = pstrdup("counted");
/* Establish signal handlers before unblocking signals. */
- pqsignal(SIGHUP, worker_spi_sighup);
- pqsignal(SIGTERM, worker_spi_sigterm);
+ pqsignal(SIGHUP, SignalHandlerForConfigReload);
+ pqsignal(SIGTERM, SignalHandlerForShutdownRequest);
/* We're now ready to receive signals */
BackgroundWorkerUnblockSignals();
@@ -221,7 +186,7 @@ worker_spi_main(Datum main_arg)
/*
* Main loop: do this until the SIGTERM handler tells us to terminate
*/
- while (!got_sigterm)
+ while (!ShutdownRequestPending)
{
int ret;
@@ -242,9 +207,9 @@ worker_spi_main(Datum main_arg)
/*
* In case of a SIGHUP, just reload the configuration.
*/
- if (got_sighup)
+ if (ConfigReloadPending)
{
- got_sighup = false;
+ ConfigReloadPending = false;
ProcessConfigFile(PGC_SIGHUP);
}
--
2.25.1
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-29 06:21 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 09:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 15:46 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-05 03:12 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Kyotaro Horiguchi <[email protected]>
2020-11-06 17:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-07 14:01 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-10 07:17 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Kyotaro Horiguchi <[email protected]>
2020-11-10 09:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-10 12:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
@ 2020-11-12 04:36 ` Fujii Masao <[email protected]>
2020-11-13 11:24 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
1 sibling, 1 reply; 38+ messages in thread
From: Fujii Masao @ 2020-11-12 04:36 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; pgsql-hackers
On 2020/11/10 21:30, Bharath Rupireddy wrote:
> On Tue, Nov 10, 2020 at 3:04 PM Fujii Masao <[email protected]> wrote:
>>
>>>> The main reason for having SetLatch() in
>>>> SignalHandlerForConfigReload() is to wake up the calling process if
>>>> waiting in WaitLatchOrSocket() or WaitLatch() and reload the new
>>>> config file and use the reloaded config variables. Maybe we should
>>>> give a thought on the scenarios in which the walreceiver process
>>>> waits, and what happens in case the latch is set when SIGHUP is
>>>> received.
>>>
>>> The difference is whether the config file is processed at the next
>>> wakeup (by force-reply-request or SIGTERM) of walreceiver or
>>> immediately. If server-reload happened frequently, say, several times
>>> per second(?), we should consider to reduce the useless reloading, but
>>> actually that's not the case.
>>
>> So, attached is the patch that makes walreceiver use both standard
>> SIGTERM and SIGHUP handlers. Currently I've not found any actual
>> issues by making walreceiver use standard SIGHUP handler, yet.
>>
>
> I think it makes sense to replace WalRcv->latch with
> MyProc->procLatch(as they point to the same latch) in the functions
> that are called in the walreceiver process. However, we are using
> walrcv->latch with spinlock, say in WalRcvForceReply() and
> RequestXLogStreaming() both are called from the startup process. See
> commit 45f9d08684, that made the access to the walreceiver's
> latch(WalRcv->latch) by the other process(startup) spinlock protected
>
> And looks like, in general it's a standard practice to set latch to
> wake up the process if waiting in case a SIGHUP signal is received and
> reload the relevant config variables.
>
> Going by the above analysis, the v3 patch looks good to me.
Thanks for the analysis! I pushed the patch.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-29 06:21 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 09:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 15:46 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-05 03:12 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Kyotaro Horiguchi <[email protected]>
2020-11-06 17:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-07 14:01 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-10 07:17 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Kyotaro Horiguchi <[email protected]>
2020-11-10 09:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-10 12:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-12 04:36 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
@ 2020-11-13 11:24 ` Bharath Rupireddy <[email protected]>
2020-11-17 08:16 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Bharath Rupireddy @ 2020-11-13 11:24 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; pgsql-hackers
On Thu, Nov 12, 2020 at 10:06 AM Fujii Masao
<[email protected]> wrote:
>
> Thanks for the analysis! I pushed the patch.
>
Thanks! Since we are replacing custom SIGHUP and SIGTERM handlers with
standard ones, how about doing the same thing in worker_spi.c? I
posted a patch previously [1] in this mail thread. If it makes sense,
please review it.
[1] - https://www.postgresql.org/message-id/CALj2ACXDEZhAFOTDcqO9cFSRvrFLyYOnPKrcA1UG4uZn9hUAVg%40mail.gma...
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-29 06:21 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 09:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 15:46 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-05 03:12 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Kyotaro Horiguchi <[email protected]>
2020-11-06 17:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-07 14:01 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-10 07:17 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Kyotaro Horiguchi <[email protected]>
2020-11-10 09:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-10 12:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-11-12 04:36 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-13 11:24 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
@ 2020-11-17 08:16 ` Fujii Masao <[email protected]>
0 siblings, 0 replies; 38+ messages in thread
From: Fujii Masao @ 2020-11-17 08:16 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; pgsql-hackers
On 2020/11/13 20:24, Bharath Rupireddy wrote:
> On Thu, Nov 12, 2020 at 10:06 AM Fujii Masao
> <[email protected]> wrote:
>>
>> Thanks for the analysis! I pushed the patch.
>>
>
> Thanks! Since we are replacing custom SIGHUP and SIGTERM handlers with
> standard ones, how about doing the same thing in worker_spi.c? I
> posted a patch previously [1] in this mail thread. If it makes sense,
> please review it.
I agree to simplify the worker_spi code by making it use the standard
signal handlers. But as far as I read Craig Ringer's comments upthread
about worker_spi, it's not enough to just replace the dedicated SIGTERM
handler with the standard one. ISTM that probably worker_spi should
use the signal handler handling InterruptPending and ProcDiePending
like die() does. What do you think about Craig Ringer's comments?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-29 06:21 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 09:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
@ 2020-12-15 14:10 ` Fujii Masao <[email protected]>
2020-12-16 07:24 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Kyotaro Horiguchi <[email protected]>
1 sibling, 1 reply; 38+ messages in thread
From: Fujii Masao @ 2020-12-15 14:10 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: pgsql-hackers
On 2020/11/04 18:06, Fujii Masao wrote:
>
>
> On 2020/10/29 15:21, Fujii Masao wrote:
>>
>>
>> On 2020/10/07 11:30, Bharath Rupireddy wrote:
>>> On Tue, Oct 6, 2020 at 11:41 AM Bharath Rupireddy
>>> <[email protected]> wrote:
>>>>
>>>> On Tue, Oct 6, 2020 at 11:20 AM Fujii Masao <[email protected]> wrote:
>>>>>
>>>>> +1 Or it's also ok to make each patch separately.
>>>>> Anyway, thanks!
>>>>>
>>>>
>>>> Thanks. +1 to have separate patches. I will post two separate patches
>>>> for autoprewarm and WalRcvShutdownHandler for further review. The
>>>> other two patches can be for startup.c and syslogger.c.
>>>>
>>>
>>> I'm attaching all the 4 patches here together.
>>>
>>> 1. v1-use-standard-SIGHUP-and-SIGTERM-handlers-in-autoprewarm-process.patch
>>> -- This is the patch initially sent in this mail by me, I just
>>> renamed it.
>>> 2. v1-use-MyLatch-and-standard-SIGHUP-handler-in-startup-process.patch
>>> -- This is the patch proposed by Fuji Masao, I just renamed it.
>>> 3. v1-use-standard-SIGHUP-hanlder-in-syslogger-process.patch
>>> 4. v1-use-standard-SIGTERM-handler-in-walreceiver-process.patch
>>>
>>> Please consider these patches for further review.
>>
>> Thanks for the patches! They look good to me. So barring any objections,
>> I will commit them one by one.
>
> I pushed the following two patches.
>
> - v1-use-standard-SIGHUP-hanlder-in-syslogger-process.patch
> - v1-use-MyLatch-and-standard-SIGHUP-handler-in-startup-process.patch
As I told in other thread [1], I'm thinking to revert this patch
because this change could have bad side-effect on the startup
process waiting for recovery conflict.
Before applying the patch, the latch that the startup process
used to wait for recovery conflict was different from the latch
that SIGHUP signal handler or walreceiver process, etc set to
wake the startup process up. So SIGHUP or walreceiver didn't
wake the startup process waiting for recovery conflict up unnecessary.
But the patch got rid of the dedicated latch for signaling
the startup process. This change forced us to use the same latch
to make the startup process wait or wake up. Which caused SIGHUP
signal handler or walreceiver proces to wake the startup process
waiting on the latch for recovery conflict up unnecessarily frequently.
While waiting for recovery conflict on buffer pin, deadlock needs
to be checked at least every deadlock_timeout. But that frequent
wakeups could prevent the deadlock timer from being triggered and
could delay that deadlock checks.
Therefore, I'm thinking to revert the commit ac22929a26 and 113d3591b8.
[1]
https://www.postgresql.org/message-id/[email protected]
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-29 06:21 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 09:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-12-15 14:10 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
@ 2020-12-16 07:24 ` Kyotaro Horiguchi <[email protected]>
2020-12-16 09:12 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Kyotaro Horiguchi @ 2020-12-16 07:24 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; pgsql-hackers
At Tue, 15 Dec 2020 23:10:28 +0900, Fujii Masao <[email protected]> wrote in
> > I pushed the following two patches.
> > - v1-use-standard-SIGHUP-hanlder-in-syslogger-process.patch
> > - v1-use-MyLatch-and-standard-SIGHUP-handler-in-startup-process.patch
>
> As I told in other thread [1], I'm thinking to revert this patch
> because this change could have bad side-effect on the startup
> process waiting for recovery conflict.
>
> Before applying the patch, the latch that the startup process
> used to wait for recovery conflict was different from the latch
> that SIGHUP signal handler or walreceiver process, etc set to
> wake the startup process up. So SIGHUP or walreceiver didn't
> wake the startup process waiting for recovery conflict up unnecessary.
>
> But the patch got rid of the dedicated latch for signaling
> the startup process. This change forced us to use the same latch
> to make the startup process wait or wake up. Which caused SIGHUP
> signal handler or walreceiver proces to wake the startup process
> waiting on the latch for recovery conflict up unnecessarily
> frequently.
>
> While waiting for recovery conflict on buffer pin, deadlock needs
> to be checked at least every deadlock_timeout. But that frequent
> wakeups could prevent the deadlock timer from being triggered and
> could delay that deadlock checks.
I thought that spurious wakeups don't harm. But actually
ResolveRecoveryConflictWithBufferPin doesn't consider spurious
wakeups. Only the timer woke up ResolveRecoveryConflictWithBufferPin
before the patch comes. Currently SIGHUP and XLogFlush() (on
walreceiver) also wake up startup process.
For a moment I thought that ResolveRecoveryConflictWithBufferPin
should wake up at shutdown time by the old recovery latch but it's not
the case since it wakes up after all blockers go away. It seems to me
simpler to revert the patches than making the function properly handle
spurious wakeups.
> Therefore, I'm thinking to revert the commit ac22929a26 and
> 113d3591b8.
>
> [1]
> https://www.postgresql.org/message-id/[email protected]
As the result, I agree to revert them. But I think we need to add a
comment for the reason we don't use MyLatch for recovery-wakeup after
reverting them.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-29 06:21 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 09:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-12-15 14:10 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-12-16 07:24 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Kyotaro Horiguchi <[email protected]>
@ 2020-12-16 09:12 ` Fujii Masao <[email protected]>
2020-12-17 09:12 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Fujii Masao @ 2020-12-16 09:12 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; pgsql-hackers
On 2020/12/16 16:24, Kyotaro Horiguchi wrote:
> At Tue, 15 Dec 2020 23:10:28 +0900, Fujii Masao <[email protected]> wrote in
>>> I pushed the following two patches.
>>> - v1-use-standard-SIGHUP-hanlder-in-syslogger-process.patch
>>> - v1-use-MyLatch-and-standard-SIGHUP-handler-in-startup-process.patch
>>
>> As I told in other thread [1], I'm thinking to revert this patch
>> because this change could have bad side-effect on the startup
>> process waiting for recovery conflict.
>>
>> Before applying the patch, the latch that the startup process
>> used to wait for recovery conflict was different from the latch
>> that SIGHUP signal handler or walreceiver process, etc set to
>> wake the startup process up. So SIGHUP or walreceiver didn't
>> wake the startup process waiting for recovery conflict up unnecessary.
>>
>> But the patch got rid of the dedicated latch for signaling
>> the startup process. This change forced us to use the same latch
>> to make the startup process wait or wake up. Which caused SIGHUP
>> signal handler or walreceiver proces to wake the startup process
>> waiting on the latch for recovery conflict up unnecessarily
>> frequently.
>>
>> While waiting for recovery conflict on buffer pin, deadlock needs
>> to be checked at least every deadlock_timeout. But that frequent
>> wakeups could prevent the deadlock timer from being triggered and
>> could delay that deadlock checks.
>
> I thought that spurious wakeups don't harm. But actually
> ResolveRecoveryConflictWithBufferPin doesn't consider spurious
> wakeups. Only the timer woke up ResolveRecoveryConflictWithBufferPin
> before the patch comes. Currently SIGHUP and XLogFlush() (on
> walreceiver) also wake up startup process.
>
> For a moment I thought that ResolveRecoveryConflictWithBufferPin
> should wake up at shutdown time by the old recovery latch but it's not
> the case since it wakes up after all blockers go away. It seems to me
> simpler to revert the patches than making the function properly handle
> spurious wakeups.
>
>> Therefore, I'm thinking to revert the commit ac22929a26 and
>> 113d3591b8.
>>
>> [1]
>> https://www.postgresql.org/message-id/[email protected]
>
> As the result, I agree to revert them. But I think we need to add a
> comment for the reason we don't use MyLatch for recovery-wakeup after
> reverting them.
Agreed. Attached is the patch that reverts those patches and
adds the comments about why both procLatch and recoveryWakeupLatch
are necessary.
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 8dd225c2e1..b1e5d2dbff 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -681,8 +681,18 @@ typedef struct XLogCtlData
* recoveryWakeupLatch is used to wake up the startup process to continue
* WAL replay, if it is waiting for WAL to arrive or failover trigger file
* to appear.
+ *
+ * Note that the startup process also uses another latch, its procLatch,
+ * to wait for recovery conflict. If we get rid of recoveryWakeupLatch for
+ * signaling the startup process in favor of using its procLatch, which
+ * comports better with possible generic signal handlers using that latch.
+ * But we should not do that because the startup process doesn't assume
+ * that it's waken up by walreceiver process or SIGHUP signal handler
+ * while it's waiting for recovery conflict. The separate latches,
+ * recoveryWakeupLatch and procLatch, should be used for inter-process
+ * communication for WAL replay and recovery conflict, respectively.
*/
- Latch *recoveryWakeupLatch;
+ Latch recoveryWakeupLatch;
/*
* During recovery, we keep a copy of the latest checkpoint record here.
@@ -5186,6 +5196,7 @@ XLOGShmemInit(void)
SpinLockInit(&XLogCtl->Insert.insertpos_lck);
SpinLockInit(&XLogCtl->info_lck);
SpinLockInit(&XLogCtl->ulsn_lck);
+ InitSharedLatch(&XLogCtl->recoveryWakeupLatch);
}
/*
@@ -6121,7 +6132,7 @@ recoveryApplyDelay(XLogReaderState *record)
while (true)
{
- ResetLatch(MyLatch);
+ ResetLatch(&XLogCtl->recoveryWakeupLatch);
/* might change the trigger file's location */
HandleStartupProcInterrupts();
@@ -6140,7 +6151,7 @@ recoveryApplyDelay(XLogReaderState *record)
elog(DEBUG2, "recovery apply delay %ld milliseconds", msecs);
- (void) WaitLatch(MyLatch,
+ (void) WaitLatch(&XLogCtl->recoveryWakeupLatch,
WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
msecs,
WAIT_EVENT_RECOVERY_APPLY_DELAY);
@@ -6469,11 +6480,11 @@ StartupXLOG(void)
}
/*
- * Advertise our latch that other processes can use to wake us up
- * if we're going to sleep during recovery.
+ * Take ownership of the wakeup latch if we're going to sleep during
+ * recovery.
*/
if (ArchiveRecoveryRequested)
- XLogCtl->recoveryWakeupLatch = &MyProc->procLatch;
+ OwnLatch(&XLogCtl->recoveryWakeupLatch);
/* Set up XLOG reader facility */
MemSet(&private, 0, sizeof(XLogPageReadPrivate));
@@ -7484,11 +7495,11 @@ StartupXLOG(void)
ResetUnloggedRelations(UNLOGGED_RELATION_INIT);
/*
- * We don't need the latch anymore. It's not strictly necessary to reset
- * it to NULL, but let's do it for the sake of tidiness.
+ * 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)
- XLogCtl->recoveryWakeupLatch = NULL;
+ DisownLatch(&XLogCtl->recoveryWakeupLatch);
/*
* We are now done reading the xlog from stream. Turn off streaming
@@ -12300,12 +12311,12 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
wait_time = wal_retrieve_retry_interval -
TimestampDifferenceMilliseconds(last_fail_time, now);
- (void) WaitLatch(MyLatch,
+ (void) WaitLatch(&XLogCtl->recoveryWakeupLatch,
WL_LATCH_SET | WL_TIMEOUT |
WL_EXIT_ON_PM_DEATH,
wait_time,
WAIT_EVENT_RECOVERY_RETRIEVE_RETRY_INTERVAL);
- ResetLatch(MyLatch);
+ ResetLatch(&XLogCtl->recoveryWakeupLatch);
now = GetCurrentTimestamp();
/* Handle interrupt signals of startup process */
@@ -12559,11 +12570,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(MyLatch,
+ (void) WaitLatch(&XLogCtl->recoveryWakeupLatch,
WL_LATCH_SET | WL_TIMEOUT |
WL_EXIT_ON_PM_DEATH,
5000L, WAIT_EVENT_RECOVERY_WAL_STREAM);
- ResetLatch(MyLatch);
+ ResetLatch(&XLogCtl->recoveryWakeupLatch);
break;
}
@@ -12735,8 +12746,7 @@ CheckPromoteSignal(void)
void
WakeupRecovery(void)
{
- if (XLogCtl->recoveryWakeupLatch)
- SetLatch(XLogCtl->recoveryWakeupLatch);
+ SetLatch(&XLogCtl->recoveryWakeupLatch);
}
/*
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index eab9c8c4ed..64af7b8707 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -37,6 +37,7 @@
/*
* 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;
@@ -48,6 +49,7 @@ static volatile sig_atomic_t in_restore_command = false;
/* Signal handlers */
static void StartupProcTriggerHandler(SIGNAL_ARGS);
+static void StartupProcSigHupHandler(SIGNAL_ARGS);
/* --------------------------------
@@ -62,7 +64,19 @@ StartupProcTriggerHandler(SIGNAL_ARGS)
int save_errno = errno;
promote_signaled = true;
- SetLatch(MyLatch);
+ 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();
errno = save_errno;
}
@@ -77,7 +91,7 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
proc_exit(1);
else
shutdown_requested = true;
- SetLatch(MyLatch);
+ WakeupRecovery();
errno = save_errno;
}
@@ -123,9 +137,9 @@ HandleStartupProcInterrupts(void)
/*
* Process any requests or signals received recently.
*/
- if (ConfigReloadPending)
+ if (got_SIGHUP)
{
- ConfigReloadPending = false;
+ got_SIGHUP = false;
StartupRereadConfig();
}
@@ -158,7 +172,7 @@ StartupProcessMain(void)
/*
* Properly accept or ignore signals the postmaster might send us.
*/
- pqsignal(SIGHUP, SignalHandlerForConfigReload); /* reload config file */
+ pqsignal(SIGHUP, StartupProcSigHupHandler); /* reload config file */
pqsignal(SIGINT, SIG_IGN); /* ignore query cancel */
pqsignal(SIGTERM, StartupProcShutdownHandler); /* request shutdown */
/* SIGQUIT handler was already set up by InitPostmasterChild */
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index 4ea3cf1f5c..92d9027776 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -519,7 +519,14 @@ ResolveRecoveryConflictWithBufferPin(void)
enable_timeouts(timeouts, 2);
}
- /* Wait to be signaled by UnpinBuffer() */
+ /*
+ * Wait to be signaled by UnpinBuffer().
+ *
+ * We assume that only UnpinBuffer() and the timeout requests established
+ * above can wake us up here. WakeupRecovery() called by walreceiver or
+ * SIGHUP signal handler, etc cannot do that because it uses the different
+ * latch from that ProcWaitForSignal() waits on.
+ */
ProcWaitForSignal(PG_WAIT_BUFFER_PIN);
/*
Attachments:
[text/plain] revert_remove_dedicated_latch_for_startup_process_v1.patch (7.0K, ../../[email protected]/2-revert_remove_dedicated_latch_for_startup_process_v1.patch)
download | inline diff:
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 8dd225c2e1..b1e5d2dbff 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -681,8 +681,18 @@ typedef struct XLogCtlData
* recoveryWakeupLatch is used to wake up the startup process to continue
* WAL replay, if it is waiting for WAL to arrive or failover trigger file
* to appear.
+ *
+ * Note that the startup process also uses another latch, its procLatch,
+ * to wait for recovery conflict. If we get rid of recoveryWakeupLatch for
+ * signaling the startup process in favor of using its procLatch, which
+ * comports better with possible generic signal handlers using that latch.
+ * But we should not do that because the startup process doesn't assume
+ * that it's waken up by walreceiver process or SIGHUP signal handler
+ * while it's waiting for recovery conflict. The separate latches,
+ * recoveryWakeupLatch and procLatch, should be used for inter-process
+ * communication for WAL replay and recovery conflict, respectively.
*/
- Latch *recoveryWakeupLatch;
+ Latch recoveryWakeupLatch;
/*
* During recovery, we keep a copy of the latest checkpoint record here.
@@ -5186,6 +5196,7 @@ XLOGShmemInit(void)
SpinLockInit(&XLogCtl->Insert.insertpos_lck);
SpinLockInit(&XLogCtl->info_lck);
SpinLockInit(&XLogCtl->ulsn_lck);
+ InitSharedLatch(&XLogCtl->recoveryWakeupLatch);
}
/*
@@ -6121,7 +6132,7 @@ recoveryApplyDelay(XLogReaderState *record)
while (true)
{
- ResetLatch(MyLatch);
+ ResetLatch(&XLogCtl->recoveryWakeupLatch);
/* might change the trigger file's location */
HandleStartupProcInterrupts();
@@ -6140,7 +6151,7 @@ recoveryApplyDelay(XLogReaderState *record)
elog(DEBUG2, "recovery apply delay %ld milliseconds", msecs);
- (void) WaitLatch(MyLatch,
+ (void) WaitLatch(&XLogCtl->recoveryWakeupLatch,
WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
msecs,
WAIT_EVENT_RECOVERY_APPLY_DELAY);
@@ -6469,11 +6480,11 @@ StartupXLOG(void)
}
/*
- * Advertise our latch that other processes can use to wake us up
- * if we're going to sleep during recovery.
+ * Take ownership of the wakeup latch if we're going to sleep during
+ * recovery.
*/
if (ArchiveRecoveryRequested)
- XLogCtl->recoveryWakeupLatch = &MyProc->procLatch;
+ OwnLatch(&XLogCtl->recoveryWakeupLatch);
/* Set up XLOG reader facility */
MemSet(&private, 0, sizeof(XLogPageReadPrivate));
@@ -7484,11 +7495,11 @@ StartupXLOG(void)
ResetUnloggedRelations(UNLOGGED_RELATION_INIT);
/*
- * We don't need the latch anymore. It's not strictly necessary to reset
- * it to NULL, but let's do it for the sake of tidiness.
+ * 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)
- XLogCtl->recoveryWakeupLatch = NULL;
+ DisownLatch(&XLogCtl->recoveryWakeupLatch);
/*
* We are now done reading the xlog from stream. Turn off streaming
@@ -12300,12 +12311,12 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
wait_time = wal_retrieve_retry_interval -
TimestampDifferenceMilliseconds(last_fail_time, now);
- (void) WaitLatch(MyLatch,
+ (void) WaitLatch(&XLogCtl->recoveryWakeupLatch,
WL_LATCH_SET | WL_TIMEOUT |
WL_EXIT_ON_PM_DEATH,
wait_time,
WAIT_EVENT_RECOVERY_RETRIEVE_RETRY_INTERVAL);
- ResetLatch(MyLatch);
+ ResetLatch(&XLogCtl->recoveryWakeupLatch);
now = GetCurrentTimestamp();
/* Handle interrupt signals of startup process */
@@ -12559,11 +12570,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(MyLatch,
+ (void) WaitLatch(&XLogCtl->recoveryWakeupLatch,
WL_LATCH_SET | WL_TIMEOUT |
WL_EXIT_ON_PM_DEATH,
5000L, WAIT_EVENT_RECOVERY_WAL_STREAM);
- ResetLatch(MyLatch);
+ ResetLatch(&XLogCtl->recoveryWakeupLatch);
break;
}
@@ -12735,8 +12746,7 @@ CheckPromoteSignal(void)
void
WakeupRecovery(void)
{
- if (XLogCtl->recoveryWakeupLatch)
- SetLatch(XLogCtl->recoveryWakeupLatch);
+ SetLatch(&XLogCtl->recoveryWakeupLatch);
}
/*
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index eab9c8c4ed..64af7b8707 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -37,6 +37,7 @@
/*
* 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;
@@ -48,6 +49,7 @@ static volatile sig_atomic_t in_restore_command = false;
/* Signal handlers */
static void StartupProcTriggerHandler(SIGNAL_ARGS);
+static void StartupProcSigHupHandler(SIGNAL_ARGS);
/* --------------------------------
@@ -62,7 +64,19 @@ StartupProcTriggerHandler(SIGNAL_ARGS)
int save_errno = errno;
promote_signaled = true;
- SetLatch(MyLatch);
+ 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();
errno = save_errno;
}
@@ -77,7 +91,7 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
proc_exit(1);
else
shutdown_requested = true;
- SetLatch(MyLatch);
+ WakeupRecovery();
errno = save_errno;
}
@@ -123,9 +137,9 @@ HandleStartupProcInterrupts(void)
/*
* Process any requests or signals received recently.
*/
- if (ConfigReloadPending)
+ if (got_SIGHUP)
{
- ConfigReloadPending = false;
+ got_SIGHUP = false;
StartupRereadConfig();
}
@@ -158,7 +172,7 @@ StartupProcessMain(void)
/*
* Properly accept or ignore signals the postmaster might send us.
*/
- pqsignal(SIGHUP, SignalHandlerForConfigReload); /* reload config file */
+ pqsignal(SIGHUP, StartupProcSigHupHandler); /* reload config file */
pqsignal(SIGINT, SIG_IGN); /* ignore query cancel */
pqsignal(SIGTERM, StartupProcShutdownHandler); /* request shutdown */
/* SIGQUIT handler was already set up by InitPostmasterChild */
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index 4ea3cf1f5c..92d9027776 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -519,7 +519,14 @@ ResolveRecoveryConflictWithBufferPin(void)
enable_timeouts(timeouts, 2);
}
- /* Wait to be signaled by UnpinBuffer() */
+ /*
+ * Wait to be signaled by UnpinBuffer().
+ *
+ * We assume that only UnpinBuffer() and the timeout requests established
+ * above can wake us up here. WakeupRecovery() called by walreceiver or
+ * SIGHUP signal handler, etc cannot do that because it uses the different
+ * latch from that ProcWaitForSignal() waits on.
+ */
ProcWaitForSignal(PG_WAIT_BUFFER_PIN);
/*
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-05 16:18 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-10-06 06:11 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-29 06:21 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-11-04 09:06 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-12-15 14:10 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
2020-12-16 07:24 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Kyotaro Horiguchi <[email protected]>
2020-12-16 09:12 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
@ 2020-12-17 09:12 ` Fujii Masao <[email protected]>
0 siblings, 0 replies; 38+ messages in thread
From: Fujii Masao @ 2020-12-17 09:12 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; pgsql-hackers
On 2020/12/16 18:12, Fujii Masao wrote:
>
>
> On 2020/12/16 16:24, Kyotaro Horiguchi wrote:
>> At Tue, 15 Dec 2020 23:10:28 +0900, Fujii Masao <[email protected]> wrote in
>>>> I pushed the following two patches.
>>>> - v1-use-standard-SIGHUP-hanlder-in-syslogger-process.patch
>>>> - v1-use-MyLatch-and-standard-SIGHUP-handler-in-startup-process.patch
>>>
>>> As I told in other thread [1], I'm thinking to revert this patch
>>> because this change could have bad side-effect on the startup
>>> process waiting for recovery conflict.
>>>
>>> Before applying the patch, the latch that the startup process
>>> used to wait for recovery conflict was different from the latch
>>> that SIGHUP signal handler or walreceiver process, etc set to
>>> wake the startup process up. So SIGHUP or walreceiver didn't
>>> wake the startup process waiting for recovery conflict up unnecessary.
>>>
>>> But the patch got rid of the dedicated latch for signaling
>>> the startup process. This change forced us to use the same latch
>>> to make the startup process wait or wake up. Which caused SIGHUP
>>> signal handler or walreceiver proces to wake the startup process
>>> waiting on the latch for recovery conflict up unnecessarily
>>> frequently.
>>>
>>> While waiting for recovery conflict on buffer pin, deadlock needs
>>> to be checked at least every deadlock_timeout. But that frequent
>>> wakeups could prevent the deadlock timer from being triggered and
>>> could delay that deadlock checks.
>>
>> I thought that spurious wakeups don't harm. But actually
>> ResolveRecoveryConflictWithBufferPin doesn't consider spurious
>> wakeups. Only the timer woke up ResolveRecoveryConflictWithBufferPin
>> before the patch comes. Currently SIGHUP and XLogFlush() (on
>> walreceiver) also wake up startup process.
>>
>> For a moment I thought that ResolveRecoveryConflictWithBufferPin
>> should wake up at shutdown time by the old recovery latch but it's not
>> the case since it wakes up after all blockers go away. It seems to me
>> simpler to revert the patches than making the function properly handle
>> spurious wakeups.
>>
>>> Therefore, I'm thinking to revert the commit ac22929a26 and
>>> 113d3591b8.
>>>
>>> [1]
>>> https://www.postgresql.org/message-id/[email protected]
>>
>> As the result, I agree to revert them. But I think we need to add a
>> comment for the reason we don't use MyLatch for recovery-wakeup after
>> reverting them.
>
> Agreed. Attached is the patch that reverts those patches and
> adds the comments about why both procLatch and recoveryWakeupLatch
> are necessary.
Pushed. Thanks!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module Fujii Masao <[email protected]>
@ 2020-10-06 05:53 ` Michael Paquier <[email protected]>
1 sibling, 0 replies; 38+ messages in thread
From: Michael Paquier @ 2020-10-06 05:53 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; pgsql-hackers
On Mon, Oct 05, 2020 at 11:34:05PM +0900, Fujii Masao wrote:
> 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?
That looks good to me. Nice cleanup.
> Probably we can also replace sigHupHandler() in syslogger.c with
> SignalHandlerForConfigReload()? This would be separate patch, though.
+1.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 38+ messages in thread
end of thread, other threads:[~2020-12-17 09:12 UTC | newest]
Thread overview: 38+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-10-05 10:45 Use standard SIGHUP and SIGTERM handlers in autoprewarm module Bharath Rupireddy <[email protected]>
2020-10-05 14:34 ` Fujii Masao <[email protected]>
2020-10-05 16:18 ` Bharath Rupireddy <[email protected]>
2020-10-06 05:50 ` Fujii Masao <[email protected]>
2020-10-06 06:11 ` Bharath Rupireddy <[email protected]>
2020-10-07 02:30 ` Bharath Rupireddy <[email protected]>
2020-10-07 12:38 ` Bharath Rupireddy <[email protected]>
2020-10-23 04:06 ` Craig Ringer <[email protected]>
2020-11-17 12:18 ` Bharath Rupireddy <[email protected]>
2020-11-18 12:22 ` Fujii Masao <[email protected]>
2020-11-20 10:33 ` Bharath Rupireddy <[email protected]>
2020-11-25 09:59 ` Fujii Masao <[email protected]>
2020-11-25 14:38 ` Bharath Rupireddy <[email protected]>
2020-11-26 14:07 ` Fujii Masao <[email protected]>
2020-11-26 15:15 ` Bharath Rupireddy <[email protected]>
2020-11-26 15:23 ` Fujii Masao <[email protected]>
2020-11-27 03:13 ` Bharath Rupireddy <[email protected]>
2020-11-27 06:56 ` Fujii Masao <[email protected]>
2020-11-27 07:47 ` Bharath Rupireddy <[email protected]>
2020-11-30 02:12 ` Fujii Masao <[email protected]>
2020-10-29 06:21 ` Fujii Masao <[email protected]>
2020-11-04 09:06 ` Fujii Masao <[email protected]>
2020-11-04 15:46 ` Bharath Rupireddy <[email protected]>
2020-11-05 03:12 ` Kyotaro Horiguchi <[email protected]>
2020-11-06 17:30 ` Fujii Masao <[email protected]>
2020-11-07 14:01 ` Bharath Rupireddy <[email protected]>
2020-11-10 07:17 ` Kyotaro Horiguchi <[email protected]>
2020-11-10 09:34 ` Fujii Masao <[email protected]>
2020-11-10 12:30 ` Bharath Rupireddy <[email protected]>
2020-11-10 13:11 ` Bharath Rupireddy <[email protected]>
2020-11-12 04:36 ` Fujii Masao <[email protected]>
2020-11-13 11:24 ` Bharath Rupireddy <[email protected]>
2020-11-17 08:16 ` Fujii Masao <[email protected]>
2020-12-15 14:10 ` Fujii Masao <[email protected]>
2020-12-16 07:24 ` Kyotaro Horiguchi <[email protected]>
2020-12-16 09:12 ` Fujii Masao <[email protected]>
2020-12-17 09:12 ` Fujii Masao <[email protected]>
2020-10-06 05:53 ` Michael Paquier <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox