On Fri, Dec 13, 2013 at 1:15 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2013-12-13 12:54:09 -0500, Tom Lane wrote:
>> Andres Freund <andres@2ndquadrant.com> writes:
>> > I wonder what to do about bgworker's bgworker_die()? I don't really see
>> > how that can be fixed without breaking the API?
>>
>> IMO it should be flushed and bgworkers should use the same die() handler
>> as every other backend, or else one like the one in worker_spi, which just
>> sets a flag for testing later.
>
> Agreed on not going forward like now, but I don't really see how they
> could usefully use die(). I think we should just mandate that every
> bgworker conneced to shared memory registers a sigterm handler - we
> could put a check into BackgroundWorkerUnblockSignals(). We should leave
> the current handler in for unconnected one though...
> bgworkers are supposed to be written as a loop around procLatch, so
> adding a !got_sigterm, probably isn't too hard.
I think the !got_sigterm thing is complete bunk. If a background
worker is running SQL queries, it really ought to honor a query cancel
or sigterm at the next CHECK_FOR_INTERRUPTS(). But the default
background worker handler for SIGUSR1 just sets the process latch, and
worker_spi's sigterm handler just sets a private variable got_sigterm.So ProcessInterrupts() will never get called, and
ifit did it
wouldn't do anything anyway. That's really pretty horrible, because it
means that the query worker_spi runs can't be interrupted short of a
SIGQUIT. So I think worker_spi is really a very bad example of how to
do this right. In the as-yet-uncommitted test-shm-mq-v1.patch, I did
this:
+static void
+handle_sigterm(SIGNAL_ARGS)
+{
+ int save_errno = errno;
+
+ if (MyProc)
+ SetLatch(&MyProc->procLatch);
+
+ if (!proc_exit_inprogress)
+ {
+ InterruptPending = true;
+ ProcDiePending = true;
+ }
+
+ errno = save_errno;
+}
...but I'm not 100% sure that's right, either.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
