Subject: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 5 Oct 2020 16:15:49 +0530

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-masked]>
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

---

From: Fujii Masao <[email-masked]>
Date: Mon, 5 Oct 2020 23:34:05 +0900

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?
> 

+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 */

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 5 Oct 2020 21:48:19 +0530

> The patch looks good to me.

Thanks.

> the general shared latch instead of its own one. POC patch attached.

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.

> 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.

---

From: Fujii Masao <[email-masked]>
Date: Tue, 6 Oct 2020 14:50:50 +0900

> 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.

> 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.

> post an updated patch soon.

+1 Or it's also ok to make each patch separately.
Anyway, thanks!

Regards,

---

From: Michael Paquier <[email-masked]>
Date: Tue, 6 Oct 2020 14:53:55 +0900

> the general shared latch instead of its own one. POC patch attached.

That looks good to me.  Nice cleanup.

> SignalHandlerForConfigReload()? This would be separate patch, though.

+1.
--
Michael

---

From: Bharath Rupireddy <[email-masked]>
Date: Tue, 6 Oct 2020 11:41:18 +0530

> +1 Or it's also ok to make each patch separately.

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.

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 7 Oct 2020 08:00:32 +0530

> 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.

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 7 Oct 2020 18:08:58 +0530

> 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.

---

From: Craig Ringer <[email-masked]>
Date: Fri, 23 Oct 2020 12:06:50 +0800

> > > 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).

---

From: Fujii Masao <[email-masked]>
Date: Thu, 29 Oct 2020 15:21:57 +0900

> 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,

---

From: Fujii Masao <[email-masked]>
Date: Wed, 4 Nov 2020 18:06:22 +0900

> 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.
  */

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 4 Nov 2020 21:16:29 +0530

> ResetLatch(), like other code does. Attached are the updated versions

+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.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Thu, 05 Nov 2020 12:12:06 +0900 (JST)

At Wed, 4 Nov 2020 21:16:29 +0530, Bharath Rupireddy <[email-masked]> wrote in 
> module, and the patch looks good to me.

Looks good to me, too.

> acquire the latch pointer and . Others may have better thoughts on

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

---

From: Fujii Masao <[email-masked]>
Date: Sat, 7 Nov 2020 02:30:44 +0900

>> module, and the patch looks good to me.

Thanks for the review! I pushed the patch.

> 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,

---

From: Bharath Rupireddy <[email-masked]>
Date: Sat, 7 Nov 2020 19:31:21 +0530

> I'm not sure why SetLatch() was not called in walreceiver's SIGHUP

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.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 10 Nov 2020 16:17:40 +0900 (JST)

At Sat, 7 Nov 2020 19:31:21 +0530, Bharath Rupireddy <[email-masked]> wrote in 
> waits, and what happens in case the latch is set when SIGHUP is

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.

> 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.

---

From: Fujii Masao <[email-masked]>
Date: Tue, 10 Nov 2020 18:34:44 +0900

> 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)
 					XLogWalRcvSendHSFeedback(true);
@@ -510,7 +499,7 @@ WalReceiverMain(void)
@@ -518,7 +507,7 @@ WalReceiverMain(void)
 
 					if (walrcv->force_reply)
@@ -669,7 +658,7 @@ WalRcvWaitForStartPosition(XLogRecPtr *startpoint, TimeLineID *startpointTLI)
 	WakeupRecovery();
 	for (;;)
 	{
-		ResetLatch(walrcv->latch);
+		ResetLatch(MyLatch);

@@ -701,7 +690,7 @@ WalRcvWaitForStartPosition(XLogRecPtr *startpoint, TimeLineID *startpointTLI)
 		}
 		SpinLockRelease(&walrcv->mutex);

@@ -806,28 +795,6 @@ WalRcvDie(int code, Datum arg)
 	WakeupRecovery();
 }
 
-WalRcvSigHupHandler(SIGNAL_ARGS)

---

From: Bharath Rupireddy <[email-masked]>
Date: Tue, 10 Nov 2020 18:00:07 +0530

> 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.

---

From: Bharath Rupireddy <[email-masked]>
Date: Tue, 10 Nov 2020 18:41:12 +0530

Hi,

Attaching a patch that replaces custom signal handlers for SIGHUP and
SIGTERM in worker_spi.c.

Thoughts?

With Regards,
Bharath Rupireddy.

---

From: Fujii Masao <[email-masked]>
Date: Thu, 12 Nov 2020 13:36:07 +0900

> Going by the above analysis, the v3 patch looks good to me.

Thanks for the analysis! I pushed the patch.

Regards,

---

From: Bharath Rupireddy <[email-masked]>
Date: Fri, 13 Nov 2020 16:54:28 +0530

> 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.gmail.com

With Regards,
Bharath Rupireddy.

---

From: Fujii Masao <[email-masked]>
Date: Tue, 17 Nov 2020 17:16:52 +0900

> posted a patch previously [1] in this mail thread. If it makes sense,

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,

---

From: Bharath Rupireddy <[email-masked]>
Date: Tue, 17 Nov 2020 17:48:38 +0530

Thanks Craig!

On Fri, Oct 23, 2020 at 9:37 AM Craig Ringer
<[email-masked]> 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.

---

From: Fujii Masao <[email-masked]>
Date: Wed, 18 Nov 2020 21:22:50 +0900

> 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.

> worker_spi_sighup() with standard SignalHandlerForConfigReload().

This patch also looks good to me.

> SignalHandlerForCrashExit() as with all other bg workers.

I think you're right.

Regards,

---

From: Bharath Rupireddy <[email-masked]>
Date: Fri, 20 Nov 2020 16:03:50 +0530

Thanks.

> BTW, I tried to find the past discussion about why die() was not used,

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();

Thanks.

With Regards,
Bharath Rupireddy.

---

From: Fujii Masao <[email-masked]>
Date: Wed, 25 Nov 2020 18:59:25 +0900

On 2020/11/20 19:33, Bharath Rupireddy wrote:
> On Wed, Nov 18, 2020 at 5:52 PM Fujii Masao <[email-masked] <mailto:[email-masked]>> 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.

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?

> I think the commit 2505ce0 that altered the comment has something:

Thanks for the info! Will check that.

Regards,

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 25 Nov 2020 20:08:57 +0530

On Wed, Nov 25, 2020 at 3:29 PM Fujii Masao <[email-masked]>
wrote:
> > On Wed, Nov 18, 2020 at 5:52 PM Fujii Masao <[email-masked]
> 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")));

> 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.

> do back-patch for now. Thought?

+1 to not backpatch it.

With Regards,
Bharath Rupireddy.

---

From: Fujii Masao <[email-masked]>
Date: Thu, 26 Nov 2020 23:07:42 +0900

On 2020/11/25 23:38, Bharath Rupireddy wrote:
> On Wed, Nov 25, 2020 at 3:29 PM Fujii Masao <[email-masked] <mailto:[email-masked]>> wrote:
>  >
>  > > On Wed, Nov 18, 2020 at 5:52 PM Fujii Masao <[email-masked] <mailto:[email-masked]> <mailto:[email-masked] <mailto:[email-masked]>>> 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.
>  >
> 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!

>              /* Currently there is only one non-retryable recovery conflict */
>              Assert(RecoveryConflictReason == PROCSIG_RECOVERY_CONFLICT_DATABASE);
>                       errmsg("terminating connection due to conflict with recovery"),
> *       else if (IsBackgroundWorker)
>         {
>                 ereport(FATAL,
>                              errmsg("terminating background worker \"%s\" due to administrator command",
>                              MyBgworkerEntry->bgw_type)));
>         }*
>          else
>              ereport(FATAL,
>                       errmsg("terminating connection due to administrator command")));
> 
>  >
> 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=[email-masked]

[2]
https://postgr.es/m/CAEepm=[email-masked]

Regards,

---

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 26 Nov 2020 20:45:29 +0530

> 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.

---

From: Fujii Masao <[email-masked]>
Date: Fri, 27 Nov 2020 00:23:23 +0900

On 2020/11/27 0:15, Bharath Rupireddy wrote:
> 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,

---

From: Bharath Rupireddy <[email-masked]>
Date: Fri, 27 Nov 2020 08:43:34 +0530

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.gmail.com

With Regards,
Bharath Rupireddy.

---

From: Fujii Masao <[email-masked]>
Date: Fri, 27 Nov 2020 15:56:52 +0900

On 2020/11/27 12:13, Bharath Rupireddy wrote:
> On Wed, Nov 25, 2020 at 8:08 PM Bharath Rupireddy <[email-masked] <mailto:[email-masked]>> wrote:
>  >
> 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()

Yeah, I pushed them. Thanks!

Regards,

---

From: Bharath Rupireddy <[email-masked]>
Date: Fri, 27 Nov 2020 13:17:38 +0530

Thanks!

With Regards,
Bharath Rupireddy.

---

From: Fujii Masao <[email-masked]>
Date: Mon, 30 Nov 2020 11:12:13 +0900

>> Thanks for the patch! It looks good to me.

Pushed. Thanks!

Since all the patches that proposed in this thread were committed,
I marked the CF entry as committed.

Regards,

---

From: Fujii Masao <[email-masked]>
Date: Tue, 15 Dec 2020 23:10:28 +0900

> - 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-masked]

Regards,

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Wed, 16 Dec 2020 16:24:59 +0900 (JST)

At Tue, 15 Dec 2020 23:10:28 +0900, Fujii Masao <[email-masked]> wrote in 
> 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.

> https://www.postgresql.org/message-id/[email-masked]

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.

---

From: Fujii Masao <[email-masked]>
Date: Wed, 16 Dec 2020 18:12:39 +0900

> comment for the reason we don't use MyLatch for recovery-wakeup after

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);
 
 	/*

---

From: Fujii Masao <[email-masked]>
Date: Thu, 17 Dec 2020 18:12:39 +0900

Pushed. Thanks!

Regards,