Re: common signal handler protection

On Wed, Feb 07, 2024 at 10:40:50AM -0800, Andres Freund wrote:
> On 2024-02-07 11:15:54 -0600, Nathan Bossart wrote:
>> Perhaps we should add a file global bool that is only set during
>> wrapper_handler(). Then we could Assert() or elog(ERROR, ...) if
>> pqsignal() is called with it set.
>
> In older branches that might have been harder (due to forking from a signal
> handler and non-fatal errors thrown from signal handlers), but these days I
> think that should work.
>
> FWIW, I don't think elog(ERROR) would be appropriate, that'd be jumping out of
> a signal handler :)

*facepalm* Yes.

> If it were just for the purpose of avoiding the issue you brought up it might
> not quite be worth it - but there are a lot of things we want to forbid in a
> signal handler. Memory allocations, acquiring locks, throwing non-panic
> errors, etc. That's one of the main reasons I like a common wrapper signal
> handler.
>
> Which reminded me of https://postgr.es/m/87msstvper.fsf%40163.com - the set of
> things we want to forbid are similar. I'm not sure there's really room to
> harmonize things, but I thought I'd raise it.
>
> Perhaps we should make the state a bitmap and have a single
> AssertNotInState(HOLDING_SPINLOCK | IN_SIGNALHANDLER)

Seems worth a try. I'll go ahead and proceed with these patches and leave
this improvement for another thread.

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