Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

On Wed, Nov 18, 2020 at 5:52 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com>
wrote:
>
> > handle_sigterm() and die() are similar except that die() has extra
> > handling(below) for exiting immediately when waiting for input/command
> > from the client.
> > /*
> > * If we're in single user mode, we want to quit immediately - we
can't
> > * rely on latches as they wouldn't work when stdin/stdout is a
file.
> > * Rather ugly, but it's unlikely to be worthwhile to invest much
more
> > * effort just for the benefit of single user mode.
> > */
> > if (DoingCommandRead && whereToSendOutput != DestRemote)
> > ProcessInterrupts();
> >
> > Having this extra handling is correct for normal backends as they can
> > connect directly to clients for reading input commands, the above if
> > condition may become true and ProcessInterrupts() may be called to
> > handle the SIGTERM immediately.
> >
> > Note that DoingCommandRead can never be true in bg workers as it's
> > being set to true only in normal backend PostgresMain(). If we use
> > die()(instead of custom SIGTERM handlers such as handle_sigterm()) in
> > a bg worker/non-backend process, there are no chances that the above
> > part of the code gets hit and the interrupts are processed
> > immediately. And also here are the bg worker process that use die()
> > instead of their own custom handlers: parallel workers, autoprewarm
> > worker, autovacuum worker, logical replication launcher and apply
> > worker, wal sender process
> >
> > I think we can also use die() instead of handle_sigterm() in
> > test_shm_mq/worker.c.
> >
> > I attached a patch for this change.
>
> Thanks for the patch! It looks good to me.
>

Thanks.

>
> BTW, I tried to find the past discussion about why die() was not used,
> but I could not yet.
>

I think the commit 2505ce0 that altered the comment has something:
handle_sigterm() is stripped down to remove if (DoingCommandRead &&
whereToSendOutput != DestRemote) part from die() since test_shm_mq bg
worker or for that matter any bg worker do not have an equivalent of the
regular backend's command-read loop.

--- a/src/test/modules/test_shm_mq/worker.c
+++ b/src/test/modules/test_shm_mq/worker.c
@@ -60,13 +60,9 @@ test_shm_mq_main(Datum main_arg)
*
* We want CHECK_FOR_INTERRUPTS() to kill off this worker process
just as
* it would a normal user backend. To make that happen, we
establish a
- * signal handler that is a stripped-down version of die(). We
don't have
- * any equivalent of the backend's command-read loop, where
interrupts can
- * be processed immediately, so make sure ImmediateInterruptOK is
turned
- * off.
+ * signal handler that is a stripped-down version of die().
*/
pqsignal(SIGTERM, handle_sigterm);
- ImmediateInterruptOK = false;
BackgroundWorkerUnblockSignals();

>
> > Attaching another patch that has replaced custom SIGTERM handler
> > worker_spi_sigterm() with die() and custom SIGHUP handler
> > worker_spi_sighup() with standard SignalHandlerForConfigReload().
>
> This patch also looks good to me.
>

Thanks.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com