Re: Weird failure with latches in curculio on v15

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