Horton

Re: Weird failure with latches in curculio on v15

Robert Haas <robertmhaas@gmail.com>

View thread View on postgr.es
From: Robert Haas <robertmhaas@gmail.com>
To: Nathan Bossart <nathandbossart@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>
Date: 2023-02-02T21:14:54Z
Lists: pgsql-hackers 
On Thu, Feb 2, 2023 at 3:10 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> Hm.  I don't know if we want to encourage further use of
> in_restore_command since it seems to be prone to misuse.  Here's a v2 that
> demonstrateѕ Tom's idea (bikeshedding on names and comments is welcome).  I
> personally like this approach a bit more.

+       /*
+        * When exitOnSigterm is set and we are in the startup process, use the
+        * special wrapper for system() that enables exiting immediately upon
+        * receiving SIGTERM.  This ensures we can break out of system() if
+        * required.
+        */

This comment, for me, raises more questions than it answers. Why do we
only do this in the startup process?

Also, and this part is not the fault of this patch but a defect of the
pre-existing comments, under what circumstances do we not want to exit
when we get a SIGTERM? It's standard behavior for PostgreSQL backends
to exit when they receive SIGTERM, so the question isn't why we
sometimes exit immediately but why we ever don't. The existing code
calls ExecuteRecoveryCommand with exitOnSigterm true in some cases and
false in other cases, and AFAICS there are zero words of comments
explaining the reasoning.

+       if (exitOnSigterm && MyBackendType == B_STARTUP)
+               rc = RunInterruptibleShellCommand(command);
+       else
+               rc = system(command);

And this looks like pure magic. I'm all in favor of not relying on
system(), but using it under some opaque set of conditions and
otherwise doing something else is not the way. At the very least this
needs to be explained a whole lot better.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Commits

  1. Avoid calling proc_exit() in processes forked by system().

  2. Move extra code out of the Pre/PostRestoreCommand() section.

  3. Revert refactoring of restore command code to shell_restore.c

  4. Refactor code in charge of running shell-based recovery commands

  5. Clean up inconsistent use of fflush().

  6. Report wait events for local shell commands like archive_command.