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
