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