Hi,
On 2024-02-07 11:15:54 -0600, Nathan Bossart wrote:
> On Wed, Feb 07, 2024 at 11:06:50AM -0600, Nathan Bossart wrote:
> > I'd like to get this committed (to HEAD only) in the next few weeks. TBH
> > I'm not wild about the weird caveats (e.g., race conditions when pqsignal()
> > is called within a signal handler), but I also think it is unlikely that
> > they cause any issues in practice. Please do let me know if you have any
> > concerns about this.
I don't.
> 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 :)
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)
Greetings,
Andres Freund
