On Thu, Feb 02, 2023 at 04:14:54PM -0500, Robert Haas wrote: > + /* > + * 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?
Currently, this functionality only exists in the startup process because it is only used for restore_command. More below... > 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. I've been digging into the history here. This e-mail seems to have the most context [0]. IIUC this was intended to prevent "fast" shutdowns from escalating to "immediate" shutdowns because the restore command died unexpectedly. This doesn't apply to archive_cleanup_command because we don't FATAL if it dies unexpectedly. It seems like this idea should apply to recovery_end_command, too, but AFAICT it doesn't use the same approach. My guess is that this hasn't come up because it's less likely that both 1) recovery_end_command is used and 2) someone initiates shutdown while it is running. BTW the relevant commits are cdd46c7 (added SIGTERM handling for restore_command), 9e403c2 (added recovery_end_command), and c21ac0b (added what is today called archive_cleanup_command). > + 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. If we applied this exit-on-SIGTERM behavior to recovery_end_command, I think we could combine failOnSignal and exitOnSigterm into one flag, and then it might be a little easier to explain what is going on. In any case, I agree that this deserves a lengthy explanation, which I'll continue to work on. [0] https://postgr.es/m/499047FE.9090407%40enterprisedb.com -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
