public inbox for [email protected]  

help / color / mirror / Atom feed

From: Bharath Rupireddy <[email protected]>
To: Fujii Masao <[email protected]>
Cc: Craig Ringer <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Cc: Robert Haas <[email protected]>
Subject: Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
Date: Fri, 20 Nov 2020 16:03:50 +0530
Message-ID: <CALj2ACUcBA2EK8Ey=Y+ugaRge0Ws2Fszwn-xhPsxt-FbzNiboQ@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CALj2ACXPorUqePswDtOeM_s82v9RW32E1fYmOPZ5NuE+TWKj_A@mail.gmail.com>
	<[email protected]>
	<CALj2ACV9NrMDT5Ndpm5STUYRBfKQJoPOgA5a27a1XRc4h8wVYw@mail.gmail.com>
	<[email protected]>
	<CALj2ACXoCsVFFMi4RAPE-OpWOegS8u5+y_DCP3TXi7GeG61gyg@mail.gmail.com>
	<CALj2ACX0JkSM2BhiLog1xP9x2tsdjVdL8U-=OABf2YzYS+ywpQ@mail.gmail.com>
	<CALj2ACVCnHLdSRSZJ9nkPABcLGWm=Bvj11ajuAhgU6canRJsqA@mail.gmail.com>
	<CAGRY4nxsAe_1k_9g5b47orA0S011iBoHsXHFMH7cg7HV0O1bwQ@mail.gmail.com>
	<CALj2ACWWy1YcngpCUn09AsXMfOzwjfNqbVosfoRY0vhhVWhVBw@mail.gmail.com>
	<[email protected]>

On Wed, Nov 18, 2020 at 5:52 PM Fujii Masao <[email protected]>
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

view thread (38+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
  In-Reply-To: <CALj2ACUcBA2EK8Ey=Y+ugaRge0Ws2Fszwn-xhPsxt-FbzNiboQ@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox