Thread: Signals in a BGW
I'm curious about handling signals in a background worker. As I understand it, these are blocked when the BGW is born, until enabled with BackgroundWorkerUnblockSignals() after possible customization. Is there a known, default behavior that some signals will have, if I simply BackgroundWorkerUnblockSignals() without customizing? Will SIGINT and SIGTERM, for example, have default handlers that interact with the volatile flags in miscadmin.h that are used by CHECK_FOR_INTERRUPTS, and set the process latch? I notice that the worker_spi example code customizes SIGHUP and SIGTERM, giving them handlers that set a (different, local to the module) volatile flag, and set the process latch. The example code does call CHECK_FOR_INTERRUPTS, though I assume this won't detect ProcDie at all, now that the custom SIGTERM handler is setting a different, local flag. Perhaps it still does something useful with QueryCancel? Does this use of a custom SIGTERM handler, setting a custom flag, setting the process latch, and then checked in custom code, accomplish something important that would not be accomplished by a generic handler and CHECK_FOR_INTERRUPTS ? I'm just not clearly grasping the reason it's customized here. -Chap
On 5 December 2017 at 00:40, Chapman Flack wrote:
>
> Is there a known, default behavior that some signals will
> have, if I simply BackgroundWorkerUnblockSignals() without customizing?
> Will SIGINT and SIGTERM, for example, have default handlers that
> interact with the volatile flags in miscadmin.h that are used by
> CHECK_FOR_INTERRUPTS, and set the process latch?
>
>
The default handler is bgworker_die ; see src/backend/postmaster/bgworker.c
. I don't know if elog() is safe in a signal handler, but I guess in the
absence of non-reentrant errcontext functions...
pglogical sets up its own handler 'handle_sigterm'. However, it now does
much the same as src/backend/tcop/postgres.c's 'die' function, just without
the single-user mode checks.
void
handle_sigterm(SIGNAL_ARGS)
{
int save_errno = errno;
SetLatch(MyLatch);
if (!proc_exit_inprogress)
{
InterruptPending = true;
ProcDiePending = true;
}
errno = save_errno;
}
so it's not significantly different. We used to have our own signal
handling but it gets seriously messy when you're calling into arbitrary
PostgreSQL code that expects CHECK_FOR_INTERRUPTS() to work.
> I notice that the worker_spi example code customizes SIGHUP
> and SIGTERM, giving them handlers that set a (different, local
> to the module) volatile flag, and set the process latch.
>
IMO it's silly to customise them, and a bad example.
Personally I'd rather change the default bgw handler to 'die', make the
sample bgworkers use CHECK_FOR_INTERRUPTS() properly, and be done with it.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Tue, Dec 5, 2017 at 10:03 AM, Craig Ringer <craig@2ndquadrant.com> wrote: > pglogical sets up its own handler 'handle_sigterm'. However, it now does > much the same as src/backend/tcop/postgres.c's 'die' function, just without > the single-user mode checks. Documentation shows a simple example of that: https://www.postgresql.org/docs/devel/static/source-conventions.html > IMO it's silly to customise them, and a bad example. I don't agree. It is not silly to have background workers being able to take clean up actions and have their own tracking with some sig_atomic_t flags. -- Michael
On 12/04/2017 08:03 PM, Craig Ringer wrote: > pglogical sets up its own handler 'handle_sigterm'. However, it now does > much the same as src/backend/tcop/postgres.c's 'die' function. ... > We used to have our own signal> handling but it gets seriously messy when you're calling into arbitrary > PostgreSQL code that expects CHECK_FOR_INTERRUPTS() to work. > > ... > Personally I'd rather change the default bgw handler to 'die', make the > sample bgworkers use CHECK_FOR_INTERRUPTS() properly, and be done Short of reaching consensus to change the default bgw handler to 'die', am I right to think any bgw that wanted to could set its own SIGTERM handler to 'die' (its default SIGINT handler already being the normal StatementCancelHandler), and use CHECK_FOR_INTERRUPTS(), and be cool? > The default handler is bgworker_die ; see src/backend/postmaster > /bgworker.c > . I don't know if elog() is safe in a signal handler, but I guess in > the absence of non-reentrant errcontext functions... That does seem bold, doesn't it? I see there's a direct ereport(ERROR in the standard FloatExceptionHandler also. Does that get exercised much? I tried a quick select '1.0'::float8 / '0.0'::float8; but got a more-specific 22012 division by zero, so it looks like such things are mostly checked early and SIGFPE should be rare. -Chap
On Wed, Dec 6, 2017 at 6:59 PM, Chapman Flack <chap@anastigmatix.net> wrote: >> The default handler is bgworker_die ; see src/backend/postmaster >> /bgworker.c >> . I don't know if elog() is safe in a signal handler, but I guess in >> the absence of non-reentrant errcontext functions... > > That does seem bold, doesn't it? Yes, I think it's flat busted. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2017-12-07 12:35:07 -0500, Robert Haas wrote: > On Wed, Dec 6, 2017 at 6:59 PM, Chapman Flack <chap@anastigmatix.net> wrote: > >> The default handler is bgworker_die ; see src/backend/postmaster > >> /bgworker.c > >> . I don't know if elog() is safe in a signal handler, but I guess in > >> the absence of non-reentrant errcontext functions... > > > > That does seem bold, doesn't it? > > Yes, I think it's flat busted. We've had that discussion a couple times. The concensus so far has been that FATALing is considered kinda ok, everything else not. But it definitely has caused issues in the ast, mostly due to malloc being entered while already in malloc. Greetings, Andres Freund
On 12/07/2017 02:58 PM, Andres Freund wrote: > On 2017-12-07 12:35:07 -0500, Robert Haas wrote: >> On Wed, Dec 6, 2017 at 6:59 PM, Chapman Flack <chap@anastigmatix.net> wrote: >>>> The default handler is bgworker_die ; see src/backend/postmaster >>>> /bgworker.c >>>> . I don't know if elog() is safe in a signal handler, but I guess in >>>> the absence of non-reentrant errcontext functions... >>> >>> That does seem bold, doesn't it? >> >> Yes, I think it's flat busted. > > We've had that discussion a couple times. The concensus so far has been > that FATALing is considered kinda ok, everything else not. But it > definitely has caused issues in the ast, mostly due to malloc being > entered while already in malloc. Well, bgworker.c:bgworker_die() FATALs, so that might be kinda ok, but postgres.c:FloatExceptionHandler() ERRORs, so what's up with that? Has it just not caused much trouble because the existing math operators test their operands rather than relying on exceptions, and it might only get called in case of some extension that did float operations without checking? I admit I've been trying to think of a better thing it could do, and it seems challenging ... ideally you'd want an ERROR to happen, and soon (before trying to evaluate more of the expression), from code that might not be checking right away ... but how could that be made to work? -Chap