Date: Thu, 2 Feb 2023 21:35:48 -0800
From: Nathan Bossart <nathandbossart@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>, Tom Lane <tgl@sss.pgh.pa.us>,
	Andres Freund <andres@anarazel.de>,
	Thomas Munro <thomas.munro@gmail.com>,
	Fujii Masao <fujii@postgresql.org>,
	Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Weird failure with latches in curculio on v15
Message-ID: <20230203053548.GA27055@nathanxps13>
References: <20230201165801.33ydbxvjdbomjqa7@alap3.anarazel.de>
 <20230201175806.GA3199959@nathanxps13>
 <20230201223555.GA3721373@nathanxps13>
 <Y9sam108o4mxZFiS@paquier.xyz>
 <1449633.1675305284@sss.pgh.pa.us>
 <Y9s678gkiX0pEb5C@paquier.xyz>
 <20230202200957.GA3944544@nathanxps13>
 <CA+Tgmob+KZQn_EfVOp9umWc6iJmuzn5oH9hF_9+Cdu=wPgiEZg@mail.gmail.com>
 <20230202220113.GA3945808@nathanxps13>
 <20230202223919.GA3947443@nathanxps13>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="nFreZHaLTZJo0R7j"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230202223919.GA3947443@nathanxps13>
Archived-At: 
 <https://www.postgresql.org/message-id/20230203053548.GA27055%40nathanxps13>
Precedence: bulk


--nFreZHaLTZJo0R7j
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On Thu, Feb 02, 2023 at 02:39:19PM -0800, Nathan Bossart wrote:
> Maybe we could just
> remove this exit-in-SIGTERM-handler business...

I've spent some time testing this.  It seems to work pretty well, but only
if I keep the exit-on-SIGTERM logic in shell_restore().  Without that, I'm
seeing delayed shutdowns, which I assume means
HandleStartupProcInterrupts() isn't getting called (I'm still investigating
this).  Іn any case, the fact that shell_restore() exits if the command
fails due to SIGTERM seems like an implementation detail that we won't
necessarily want to rely on once recovery modules are available.  In short,
we seem to depend on the SIGTERM handling in RestoreArchivedFile() in order
to be responsive to shutdown requests.

One idea I have is to approximate the current behavior by simply checking
for the shutdown_requested flag before before and after executing
restore_command.  This seems to work as desired even if the exit-on-SIGTERM
logic is removed from shell_restore().  Unless there is some reason to
break out of system() (versus just waiting for the command to fail after it
receives SIGTERM), I think this approach should suffice.

I've attached a draft patch.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

--nFreZHaLTZJo0R7j
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="adjust_sigterm_handling.patch"

diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c
index 4b89addf97..56c8bf8c18 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -148,16 +148,17 @@ RestoreArchivedFile(char *path, const char *xlogfname,
 		XLogFileName(lastRestartPointFname, 0, 0L, wal_segment_size);
 
 	/*
-	 * Check signals before restore command and reset afterwards.
+	 * Check for pending shutdown requests before and after executing
+	 * restore_command and exit if there is one.
 	 */
-	PreRestoreCommand();
+	HandleStartupProcShutdownRequest();
 
 	/*
 	 * Copy xlog from archival storage to XLOGDIR
 	 */
 	ret = shell_restore(xlogfname, xlogpath, lastRestartPointFname);
 
-	PostRestoreCommand();
+	HandleStartupProcShutdownRequest();
 
 	if (ret)
 	{
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index bcd23542f1..30c67a670c 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -55,12 +55,6 @@ static volatile sig_atomic_t got_SIGHUP = false;
 static volatile sig_atomic_t shutdown_requested = false;
 static volatile sig_atomic_t promote_signaled = false;
 
-/*
- * Flag set when executing a restore command, to tell SIGTERM signal handler
- * that it's safe to just proc_exit.
- */
-static volatile sig_atomic_t in_restore_command = false;
-
 /*
  * Time at which the most recent startup operation started.
  */
@@ -120,10 +114,7 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
 {
 	int			save_errno = errno;
 
-	if (in_restore_command)
-		proc_exit(1);
-	else
-		shutdown_requested = true;
+	shutdown_requested = true;
 	WakeupRecovery();
 
 	errno = save_errno;
@@ -183,8 +174,7 @@ HandleStartupProcInterrupts(void)
 	/*
 	 * Check if we were requested to exit without finishing recovery.
 	 */
-	if (shutdown_requested)
-		proc_exit(1);
+	HandleStartupProcShutdownRequest();
 
 	/*
 	 * Emergency bailout if postmaster has died.  This is to avoid the
@@ -273,26 +263,16 @@ StartupProcessMain(void)
 	proc_exit(0);
 }
 
+/*
+ * Exit if there is a pending shutdown request.
+ */
 void
-PreRestoreCommand(void)
+HandleStartupProcShutdownRequest(void)
 {
-	/*
-	 * Set in_restore_command to tell the signal handler that we should exit
-	 * right away on SIGTERM. We know that we're at a safe point to do that.
-	 * Check if we had already received the signal, so that we don't miss a
-	 * shutdown request received just before this.
-	 */
-	in_restore_command = true;
 	if (shutdown_requested)
 		proc_exit(1);
 }
 
-void
-PostRestoreCommand(void)
-{
-	in_restore_command = false;
-}
-
 bool
 IsPromoteSignaled(void)
 {
diff --git a/src/include/postmaster/startup.h b/src/include/postmaster/startup.h
index dd957f9291..fc20eea1d4 100644
--- a/src/include/postmaster/startup.h
+++ b/src/include/postmaster/startup.h
@@ -27,8 +27,7 @@ extern PGDLLIMPORT int log_startup_progress_interval;
 
 extern void HandleStartupProcInterrupts(void);
 extern void StartupProcessMain(void) pg_attribute_noreturn();
-extern void PreRestoreCommand(void);
-extern void PostRestoreCommand(void);
+extern void HandleStartupProcShutdownRequest(void);
 extern bool IsPromoteSignaled(void);
 extern void ResetPromoteSignaled(void);
 

--nFreZHaLTZJo0R7j--