Обсуждение: Use standard die() handler for SIGTERM in bgworkers
Starting a separate thread for this change that was part of the giant "Interrupts vs signals" patch set [https://www.postgresql.org/message-id/818bafaf-1e77-4c78-8037-d7120878d87c%40iki.fi] On 14/02/2026 23:56, Andres Freund wrote: >> From 63d1a57f4906a924c426def4e1a7f27a71611b28 Mon Sep 17 00:00:00 2001 >> From: Heikki Linnakangas <heikki.linnakangas@iki.fi> >> Date: Tue, 20 Jan 2026 16:57:25 +0200 >> Subject: [PATCH 3/5] Use standard die() handler for SIGTERM in bgworkers >> -/* >> - * Standard SIGTERM handler for background workers >> - */ >> -static void >> -bgworker_die(SIGNAL_ARGS) >> -{ >> - sigprocmask(SIG_SETMASK, &BlockSig, NULL); >> - >> - ereport(FATAL, >> - (errcode(ERRCODE_ADMIN_SHUTDOWN), >> - errmsg("terminating background worker \"%s\" due to administrator command", >> - MyBgworkerEntry->bgw_type))); >> -} >> - >> /* >> * Main entry point for background worker processes. >> */ > > Uh, huh. So we were defaulting to completely unsafe code in bgworkers all this > time? This obviously can self-deadlock against memory allocations etc in the > interrupted code... Or cause confusion with the IO streams for stderr. Or ... Yep. Here's this patch again, now with updated documentation. > We really need some instrumentation that fails if we do allocations in signal > handlers etc. Yeah, that would be nice.. - Heikki
Вложения
On Tue, Feb 17, 2026 at 11:18:00PM +0200, Heikki Linnakangas wrote: > On 14/02/2026 23:56, Andres Freund wrote: >> We really need some instrumentation that fails if we do allocations in signal >> handlers etc. > > Yeah, that would be nice.. In theory we could pretty easily add assertions for that, given the wrapper_handler business added a couple of years ago. I'll put together a patch... -- nathan
On Tue, Feb 17, 2026 at 11:18:00PM +0200, Heikki Linnakangas wrote: > On 14/02/2026 23:56, Andres Freund wrote: >> Uh, huh. So we were defaulting to completely unsafe code in bgworkers all this >> time? This obviously can self-deadlock against memory allocations etc in the >> interrupted code... Or cause confusion with the IO streams for stderr. Or ... > > Yep. > > Here's this patch again, now with updated documentation. Looks reasonable to me. -- nathan
On Wed, 18 Feb 2026 at 02:18, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > > Starting a separate thread for this change that was part of the giant > "Interrupts vs signals" patch set > [https://www.postgresql.org/message-id/818bafaf-1e77-4c78-8037-d7120878d87c%40iki.fi] > > On 14/02/2026 23:56, Andres Freund wrote: > >> From 63d1a57f4906a924c426def4e1a7f27a71611b28 Mon Sep 17 00:00:00 2001 > >> From: Heikki Linnakangas <heikki.linnakangas@iki.fi> > >> Date: Tue, 20 Jan 2026 16:57:25 +0200 > >> Subject: [PATCH 3/5] Use standard die() handler for SIGTERM in bgworkers > >> -/* > >> - * Standard SIGTERM handler for background workers > >> - */ > >> -static void > >> -bgworker_die(SIGNAL_ARGS) > >> -{ > >> - sigprocmask(SIG_SETMASK, &BlockSig, NULL); > >> - > >> - ereport(FATAL, > >> - (errcode(ERRCODE_ADMIN_SHUTDOWN), > >> - errmsg("terminating background worker \"%s\" due to administrator command", > >> - MyBgworkerEntry->bgw_type))); > >> -} > >> - > >> /* > >> * Main entry point for background worker processes. > >> */ > > > > Uh, huh. So we were defaulting to completely unsafe code in bgworkers all this > > time? This obviously can self-deadlock against memory allocations etc in the > > interrupted code... Or cause confusion with the IO streams for stderr. Or ... > > Yep. > > Here's this patch again, now with updated documentation. > > > We really need some instrumentation that fails if we do allocations in signal > > handlers etc. > > Yeah, that would be nice.. > > - Heikki Hi! > + <para> > + The default signal handlers merely set interrupt flags > + that are processed later by <function>CHECK_FOR_INTERRUPTS()</function>. > + <function>CHECK_FOR_INTERRUPTS()</function> should be called in any > + long-running loop to ensure that the background worker doesn't prevent the > + system from shutting down in a timely fashion. > + </para> Unless you are holding LWLock (or, more generally, inside the CRIT section). Should we update wording for this case, or is it maybe already assumed in doc? Aside from this, patch is good, but I think elog is still reachable from signal handler for single-mode [0]. But since single-mode does not wake up any bgworker, that's not a problem here. [0] https://www.postgresql.org/message-id/CALdSSPgFNJ7HK93QbAk2K%2BVr88dtuG5t7nGaRF7S6rmcmGpTaA%40mail.gmail.com -- Best regards, Kirill Reshke
On 18/02/2026 10:45, Kirill Reshke wrote: > On Wed, 18 Feb 2026 at 02:18, Heikki Linnakangas <hlinnaka@iki.fi> wrote: >> + <para> >> + The default signal handlers merely set interrupt flags >> + that are processed later by <function>CHECK_FOR_INTERRUPTS()</function>. >> + <function>CHECK_FOR_INTERRUPTS()</function> should be called in any >> + long-running loop to ensure that the background worker doesn't prevent the >> + system from shutting down in a timely fashion. >> + </para> > > Unless you are holding LWLock (or, more generally, inside the CRIT > section). Should we update wording for this case, or is it maybe > already assumed in doc? It's OK to call CHECK_FOR_INTERRUPTS() while holding locks or in a critical section. It's a no-op in that case. I wouldn't go into the details here on how to use CHECK_FOR_INTERRUPTS(). It would be good to document all that somewhere, but it should go to some more general section on how to write server C functions as it's applicable to any C code. For background workers, I think the above is good. Pushed, thanks for the review! - Heikki