On Thu, Feb 2, 2023 at 3:10 PM Nathan Bossart <nathandboss...@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
