Re: stopgap fix for signal handling during restore_command

On Sat, Feb 25, 2023 at 11:07:42AM -0800, Andres Freund wrote:
> On 2023-02-23 20:33:23 -0800, Nathan Bossart wrote:>  
>>  	if (in_restore_command)
>> -		proc_exit(1);
>> +	{
>> +		/*
>> +		 * If we are in a child process (e.g., forked by system() in
>> +		 * RestoreArchivedFile()), we don't want to call any exit callbacks.
>> +		 * The parent will take care of that.
>> +		 */
>> +		if (MyProcPid == (int) getpid())
>> +			proc_exit(1);
>> +		else
>> +		{
>> +			const char	msg[] = "StartupProcShutdownHandler() called in child process\n";
>> +			int			rc pg_attribute_unused();
>> +
>> +			rc = write(STDERR_FILENO, msg, sizeof(msg));
>> +			_exit(1);
>> +		}
>> +	}
> 
> Why do we need that rc variable? Don't we normally get away with (void)
> write(...)?

My compiler complains about that.  :/

	../postgresql/src/backend/postmaster/startup.c: In function ‘StartupProcShutdownHandler’:
	../postgresql/src/backend/postmaster/startup.c:139:11: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result]
	  139 |    (void) write(STDERR_FILENO, msg, sizeof(msg));
	      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
	cc1: all warnings being treated as errors

>> diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
>> index 22b4278610..e3da0622d7 100644
>> --- a/src/backend/storage/lmgr/proc.c
>> +++ b/src/backend/storage/lmgr/proc.c
>> @@ -805,6 +805,7 @@ ProcKill(int code, Datum arg)
>>  	dlist_head *procgloballist;
>>  
>>  	Assert(MyProc != NULL);
>> +	Assert(MyProcPid == (int) getpid());  /* not safe if forked by system(), etc. */
>>  
>>  	/* Make sure we're out of the sync rep lists */
>>  	SyncRepCleanupAtProcExit();
>> @@ -925,6 +926,7 @@ AuxiliaryProcKill(int code, Datum arg)
>>  	PGPROC	   *proc;
>>  
>>  	Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
>> +	Assert(MyProcPid == (int) getpid());  /* not safe if forked by system(), etc. */
>>  
>>  	auxproc = &AuxiliaryProcs[proctype];
>>  
>> -- 
>> 2.25.1
> 
> I think the much more interesting assertion here would be to check that
> MyProc->pid equals the current pid.

I don't mind changing this, but why is this a more interesting assertion?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com