Hi,
On 2023-02-03 10:54:17 -0800, Nathan Bossart wrote:
> @@ -146,7 +146,25 @@ ExecuteRecoveryCommand(const char *command, const char
> *commandName,
> */
> fflush(NULL);
> pgstat_report_wait_start(wait_event_info);
> +
> + /*
> + * PreRestoreCommand() is used to tell the SIGTERM handler for the
> startup
> + * process that it is okay to proc_exit() right away on SIGTERM. This
> is
> + * done for the duration of the system() call because there isn't a good
> + * way to break out while it is executing. Since we might call
> proc_exit()
> + * in a signal handler here, it is extremely important that nothing but
> the
> + * system() call happens between the calls to PreRestoreCommand() and
> + * PostRestoreCommand(). Any additional code must go before or after
> this
> + * section.
> + */
> + if (exitOnSigterm)
> + PreRestoreCommand();
> +
> rc = system(command);
> +
> + if (exitOnSigterm)
> + PostRestoreCommand();
> +
> pgstat_report_wait_end();
>
> if (rc != 0)
It's somewhat weird that we now call the startup-process specific
PreRestoreCommand/PostRestoreCommand() in other processes than the
startup process. Gated on a variable that's not immediately obviously
tied to being in the startup process.
I think at least we ought to add comments to PreRestoreCommand /
PostRestoreCommand that they need to be robust against being called
outside of the startup process, and similarly a comment in
ExecuteRecoveryCommand(), explaining that all this stuff just works in
the startup process.
> diff --git a/src/backend/postmaster/startup.c
> b/src/backend/postmaster/startup.c
> index bcd23542f1..503eb1a5a6 100644
> --- a/src/backend/postmaster/startup.c
> +++ b/src/backend/postmaster/startup.c
> @@ -19,6 +19,8 @@
> */
> #include "postgres.h"
>
> +#include <unistd.h>
> +
> #include "access/xlog.h"
> #include "access/xlogrecovery.h"
> #include "access/xlogutils.h"
> @@ -121,7 +123,17 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
> int save_errno = errno;
>
> if (in_restore_command)
> - proc_exit(1);
> + {
> + /*
> + * If we are in a child process (e.g., forked by system() in
> + * shell_restore()), we don't want to call any exit callbacks.
> The
> + * parent will take care of that.
> + */
> + if (MyProcPid == (int) getpid())
> + proc_exit(1);
> + else
> + _exit(1);
I think it might be worth adding something like
const char msg[] = "StartupProcShutdownHandler() called in child process";
write(STDERR_FILENO, msg, sizeof(msg));
to this path. Otherwise it might end up being a very hard to debug path.
Greetings,
Andres Freund
