Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module - Mailing list pgsql-hackers

On Wed, Nov 18, 2020 at 5:52 PM Fujii Masao <masao.fujii@oss.nttdata.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;
>
> > 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