Re: stopgap fix for signal handling during restore_command

On Tue, Oct 10, 2023 at 04:40:28PM -0700, Andres Freund wrote:
> On 2023-03-01 14:47:51 -0800, Nathan Bossart wrote:
>> diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
>> index 22b4278610..b9e2c3aafe 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(MyProc->pid == (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(MyProc->pid == (int) getpid());  /* not safe if forked by system(), etc. */
>>  
>>      auxproc = &AuxiliaryProcs[proctype];
>>  
> 
> I'd make these elog(PANIC), I think. The paths are not performance critical
> enough that a single branch hurts, so the overhead of the check is irrelevant,
> and the consequences of calling ProcKill() twice for the same process are very
> severe.

Right.  Should we write_stderr_signal_safe() and then abort() to keep these
paths async-signal-safe?

>> +/*
>> + * Write a message to STDERR using only async-signal-safe functions.  This can
>> + * be used to safely emit a message from a signal handler.
>> + *
>> + * TODO: It is likely possible to safely do a limited amount of string
>> + * interpolation (e.g., %s and %d), but that is not presently supported.
>> + */
>> +void
>> +write_stderr_signal_safe(const char *fmt)
> 
> As is, this isn't a format, so I'd probably just name it s or str :)

Yup.

>> -/*
>> - * Write errors to stderr (or by equal means when stderr is
>> - * not available). Used before ereport/elog can be used
>> - * safely (memory context, GUC load etc)
>> - */
>>  extern void write_stderr(const char *fmt,...) pg_attribute_printf(1, 2);
>> +extern void write_stderr_signal_safe(const char *fmt);
> 
> Not sure why you removed the comment?

I think it was because it's an exact copy of the comment above the function
in elog.c, and I didn't want to give the impression that it applied to the
signal-safe one, too.  I added it back along with a new comment for
write_stderr_signal_safe().

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