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
