From a506a4ae932936222554c8be75b290eac57824d5 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 28 Nov 2023 15:18:13 -0600
Subject: [PATCH v3 2/3] Centralize logic for restoring errno in signal
 handlers.

Presently, we rely on each individual signal handler to save the
initial value of errno and then restore it before returning if
needed.  This is easily forgotten and, if missed, often goes
undetected for a long time.

In commit XXXXXXXXXX, we introduced a wrapper signal handler
function that checks whether MyProcPid matches getpid().  This
commit moves the aforementioned errno restoration code from the
individual signal handlers to the new wrapper handler so that we no
longer need to worry about missing it.

Reviewed-by: Andres Freund
Discussion: https://postgr.es/m/20231121212008.GA3742740%40nathanxps13
---
 src/backend/postmaster/autovacuum.c   |  4 ----
 src/backend/postmaster/checkpointer.c |  4 ----
 src/backend/postmaster/interrupt.c    |  8 --------
 src/backend/postmaster/pgarch.c       |  4 ----
 src/backend/postmaster/postmaster.c   | 16 ----------------
 src/backend/postmaster/startup.c      | 12 ------------
 src/backend/postmaster/syslogger.c    |  4 ----
 src/backend/replication/walsender.c   |  4 ----
 src/backend/storage/ipc/latch.c       |  4 ----
 src/backend/storage/ipc/procsignal.c  |  4 ----
 src/backend/tcop/postgres.c           |  8 --------
 src/backend/utils/misc/timeout.c      |  4 ----
 src/fe_utils/cancel.c                 |  3 ---
 src/port/pqsignal.c                   |  6 ++++++
 14 files changed, 6 insertions(+), 79 deletions(-)

diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 86a3b3d8be..ced7f72f63 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -1423,12 +1423,8 @@ AutoVacWorkerFailed(void)
 static void
 avl_sigusr2_handler(SIGNAL_ARGS)
 {
-	int			save_errno = errno;
-
 	got_SIGUSR2 = true;
 	SetLatch(MyLatch);
-
-	errno = save_errno;
 }
 
 
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
index 42c807d352..6d8c1b7ce4 100644
--- a/src/backend/postmaster/checkpointer.c
+++ b/src/backend/postmaster/checkpointer.c
@@ -834,15 +834,11 @@ IsCheckpointOnSchedule(double progress)
 static void
 ReqCheckpointHandler(SIGNAL_ARGS)
 {
-	int			save_errno = errno;
-
 	/*
 	 * The signaling process should have set ckpt_flags nonzero, so all we
 	 * need do is ensure that our main loop gets kicked out of any wait.
 	 */
 	SetLatch(MyLatch);
-
-	errno = save_errno;
 }
 
 
diff --git a/src/backend/postmaster/interrupt.c b/src/backend/postmaster/interrupt.c
index 6d4bd76bf8..fa7833f673 100644
--- a/src/backend/postmaster/interrupt.c
+++ b/src/backend/postmaster/interrupt.c
@@ -60,12 +60,8 @@ HandleMainLoopInterrupts(void)
 void
 SignalHandlerForConfigReload(SIGNAL_ARGS)
 {
-	int			save_errno = errno;
-
 	ConfigReloadPending = true;
 	SetLatch(MyLatch);
-
-	errno = save_errno;
 }
 
 /*
@@ -108,10 +104,6 @@ SignalHandlerForCrashExit(SIGNAL_ARGS)
 void
 SignalHandlerForShutdownRequest(SIGNAL_ARGS)
 {
-	int			save_errno = errno;
-
 	ShutdownRequestPending = true;
 	SetLatch(MyLatch);
-
-	errno = save_errno;
 }
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 46af349564..ff0958fa52 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -283,13 +283,9 @@ PgArchWakeup(void)
 static void
 pgarch_waken_stop(SIGNAL_ARGS)
 {
-	int			save_errno = errno;
-
 	/* set flag to do a final cycle and shut down afterwards */
 	ready_to_stop = true;
 	SetLatch(MyLatch);
-
-	errno = save_errno;
 }
 
 /*
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 7a5cd06c5c..1cc8fa2c43 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -2608,12 +2608,8 @@ InitProcessGlobals(void)
 static void
 handle_pm_pmsignal_signal(SIGNAL_ARGS)
 {
-	int			save_errno = errno;
-
 	pending_pm_pmsignal = true;
 	SetLatch(MyLatch);
-
-	errno = save_errno;
 }
 
 /*
@@ -2622,12 +2618,8 @@ handle_pm_pmsignal_signal(SIGNAL_ARGS)
 static void
 handle_pm_reload_request_signal(SIGNAL_ARGS)
 {
-	int			save_errno = errno;
-
 	pending_pm_reload_request = true;
 	SetLatch(MyLatch);
-
-	errno = save_errno;
 }
 
 /*
@@ -2705,8 +2697,6 @@ process_pm_reload_request(void)
 static void
 handle_pm_shutdown_request_signal(SIGNAL_ARGS)
 {
-	int			save_errno = errno;
-
 	switch (postgres_signal_arg)
 	{
 		case SIGTERM:
@@ -2723,8 +2713,6 @@ handle_pm_shutdown_request_signal(SIGNAL_ARGS)
 			break;
 	}
 	SetLatch(MyLatch);
-
-	errno = save_errno;
 }
 
 /*
@@ -2884,12 +2872,8 @@ process_pm_shutdown_request(void)
 static void
 handle_pm_child_exit_signal(SIGNAL_ARGS)
 {
-	int			save_errno = errno;
-
 	pending_pm_child_exit = true;
 	SetLatch(MyLatch);
-
-	errno = save_errno;
 }
 
 /*
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index 0e7de26bc2..83dbed86b9 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -96,32 +96,22 @@ static void StartupProcExit(int code, Datum arg);
 static void
 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();
-
-	errno = save_errno;
 }
 
 /* SIGTERM: set flag to abort redo and exit */
 static void
 StartupProcShutdownHandler(SIGNAL_ARGS)
 {
-	int			save_errno = errno;
-
 	if (in_restore_command)
 	{
 		/*
@@ -140,8 +130,6 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
 	else
 		shutdown_requested = true;
 	WakeupRecovery();
-
-	errno = save_errno;
 }
 
 /*
diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c
index 96dd03d9e0..a47838d505 100644
--- a/src/backend/postmaster/syslogger.c
+++ b/src/backend/postmaster/syslogger.c
@@ -1642,10 +1642,6 @@ RemoveLogrotateSignalFiles(void)
 static void
 sigUsr1Handler(SIGNAL_ARGS)
 {
-	int			save_errno = errno;
-
 	rotation_requested = true;
 	SetLatch(MyLatch);
-
-	errno = save_errno;
 }
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 3bc9c82389..a809c2202b 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -3245,12 +3245,8 @@ HandleWalSndInitStopping(void)
 static void
 WalSndLastCycleHandler(SIGNAL_ARGS)
 {
-	int			save_errno = errno;
-
 	got_SIGUSR2 = true;
 	SetLatch(MyLatch);
-
-	errno = save_errno;
 }
 
 /* Set up signal handlers */
diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index d8a69990b3..359f648703 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -2243,12 +2243,8 @@ GetNumRegisteredWaitEvents(WaitEventSet *set)
 static void
 latch_sigurg_handler(SIGNAL_ARGS)
 {
-	int			save_errno = errno;
-
 	if (waiting)
 		sendSelfPipeByte();
-
-	errno = save_errno;
 }
 
 /* Send one byte to the self-pipe, to wake up WaitLatch */
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index b7427906de..8e65009a89 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -638,8 +638,6 @@ CheckProcSignal(ProcSignalReason reason)
 void
 procsignal_sigusr1_handler(SIGNAL_ARGS)
 {
-	int			save_errno = errno;
-
 	if (CheckProcSignal(PROCSIG_CATCHUP_INTERRUPT))
 		HandleCatchupInterrupt();
 
@@ -683,6 +681,4 @@ procsignal_sigusr1_handler(SIGNAL_ARGS)
 		HandleRecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN);
 
 	SetLatch(MyLatch);
-
-	errno = save_errno;
 }
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index e415cf1f34..4bfa3fc009 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2970,8 +2970,6 @@ quickdie(SIGNAL_ARGS)
 void
 die(SIGNAL_ARGS)
 {
-	int			save_errno = errno;
-
 	/* Don't joggle the elbow of proc_exit */
 	if (!proc_exit_inprogress)
 	{
@@ -2993,8 +2991,6 @@ die(SIGNAL_ARGS)
 	 */
 	if (DoingCommandRead && whereToSendOutput != DestRemote)
 		ProcessInterrupts();
-
-	errno = save_errno;
 }
 
 /*
@@ -3004,8 +3000,6 @@ die(SIGNAL_ARGS)
 void
 StatementCancelHandler(SIGNAL_ARGS)
 {
-	int			save_errno = errno;
-
 	/*
 	 * Don't joggle the elbow of proc_exit
 	 */
@@ -3017,8 +3011,6 @@ StatementCancelHandler(SIGNAL_ARGS)
 
 	/* If we're still here, waken anything waiting on the process latch */
 	SetLatch(MyLatch);
-
-	errno = save_errno;
 }
 
 /* signal handler for floating point exception */
diff --git a/src/backend/utils/misc/timeout.c b/src/backend/utils/misc/timeout.c
index 8ab755d363..c9790f8cdb 100644
--- a/src/backend/utils/misc/timeout.c
+++ b/src/backend/utils/misc/timeout.c
@@ -363,8 +363,6 @@ schedule_alarm(TimestampTz now)
 static void
 handle_sig_alarm(SIGNAL_ARGS)
 {
-	int			save_errno = errno;
-
 	/*
 	 * Bump the holdoff counter, to make sure nothing we call will process
 	 * interrupts directly. No timeout handler should do that, but these
@@ -452,8 +450,6 @@ handle_sig_alarm(SIGNAL_ARGS)
 	}
 
 	RESUME_INTERRUPTS();
-
-	errno = save_errno;
 }
 
 
diff --git a/src/fe_utils/cancel.c b/src/fe_utils/cancel.c
index 10c0cd4554..bffef24d63 100644
--- a/src/fe_utils/cancel.c
+++ b/src/fe_utils/cancel.c
@@ -152,7 +152,6 @@ ResetCancelConn(void)
 static void
 handle_sigint(SIGNAL_ARGS)
 {
-	int			save_errno = errno;
 	char		errbuf[256];
 
 	CancelRequested = true;
@@ -173,8 +172,6 @@ handle_sigint(SIGNAL_ARGS)
 			write_stderr(errbuf);
 		}
 	}
-
-	errno = save_errno;			/* just in case the write changed it */
 }
 
 /*
diff --git a/src/port/pqsignal.c b/src/port/pqsignal.c
index a7e9dc49fc..7e283e2957 100644
--- a/src/port/pqsignal.c
+++ b/src/port/pqsignal.c
@@ -80,10 +80,14 @@ static volatile pqsigfunc pqsignal_handlers[PG_NSIG];
  * processes do not modify shared memory, which is often detrimental.  If the
  * check succeeds, the function originally provided to pqsignal() is called.
  * Otherwise, the default signal handler is installed and then called.
+ *
+ * This wrapper also handles restoring the value of errno.
  */
 static void
 wrapper_handler(SIGNAL_ARGS)
 {
+	int			save_errno = errno;
+
 #ifndef FRONTEND
 	Assert(MyProcPid);
 
@@ -96,6 +100,8 @@ wrapper_handler(SIGNAL_ARGS)
 #endif
 
 	(*pqsignal_handlers[postgres_signal_arg]) (postgres_signal_arg);
+
+	errno = save_errno;
 }
 
 /*
-- 
2.25.1