Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kg3k1-0005HF-I8 for pgsql-hackers@arkaria.postgresql.org; Fri, 20 Nov 2020 10:34:09 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1kg3jy-0003lp-Po for pgsql-hackers@arkaria.postgresql.org; Fri, 20 Nov 2020 10:34:06 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kg3jy-0003jb-H9 for pgsql-hackers@lists.postgresql.org; Fri, 20 Nov 2020 10:34:06 +0000 Received: from mail-pg1-x52c.google.com ([2607:f8b0:4864:20::52c]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1kg3jw-0003Uh-2k for pgsql-hackers@postgresql.org; Fri, 20 Nov 2020 10:34:06 +0000 Received: by mail-pg1-x52c.google.com with SMTP id i13so6933979pgm.9 for ; Fri, 20 Nov 2020 02:34:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=2bte7veiEr9lmyW7x125nK2N6hSRkQiqZFGY1tBs7eE=; b=Hnb+eA1dulmJfb0CjjYqIJ43cgMHet+FVqelh3h0VHnqktIhOjn2WoHjCE08OU11Wa 9RaNVCxJ0ypEati5FZ4AkZv7nSz+1o2xO9ldpfWvehqaOR65SPc4ZY7siPjuXjFXhtCe r44VLXlMsq94HVNiU0FVwKq/XWrJRbsowia701i6nbt6+Q4IM8faGr38ZfTsCBifinS6 WsBimIWEfV5dPM707XSx+y42pdOQIZDZrlTJkOP0brIzKj3W9AJQVH834vvDhPnXY4gW MB9Wh21qs77LardgSVkD7Z8yJ9TozHEE/EhIp4sq+2Zi/cyZsGKaVJlCBlR4fhpW+Hhx Lzuw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=2bte7veiEr9lmyW7x125nK2N6hSRkQiqZFGY1tBs7eE=; b=Hg1sTy3bKq58sMGXXp1R7HsQ5iBkEgQVF/Byk0+6FdlRSQvcY3dS7CzADPl/XgONph GAc0/ud1AH7eKxl8cyxHp4QIgdkSScbAc3+avsDkhaqRF8lZCxwA5f77lpddhkLlKJxg ORtdF/4noXJOhnic1MFLTrmrXyQ8mBEJQmZg3z1zdnEjEsNyL7XIrN3dRIgjosaJtnXY OGHQ4g6xX5yLsw8eg2FXxCBws5tnUmV8J/xdclg1+IkPP9bftFsyqNKmzPBRRi+wNvUT e3obscuCvtPJfV+R58+BcBmp9N7Tgo6kZQKruk48EQuzfM3ltUvV7ApGW0jeuX2x85yJ trJg== X-Gm-Message-State: AOAM532rfYQkKIrEIbQ3XQuueTV8QcniYIaFGnZDruMxcJ3QA1UTTLq0 35oRlxFkFfmfdme7cQl1nr/yGp7LY+Dh+V1WfOQ= X-Google-Smtp-Source: ABdhPJxAXqga3DR435tjMqMM7RNcjDpHFSh3Gjl0K2YyDI/UTMyHQpkVu3F34jLHH1gMzgJxs1/c5d5u4P5Dy+Lv2ao= X-Received: by 2002:a62:2a81:0:b029:18c:310f:74fe with SMTP id q123-20020a622a810000b029018c310f74femr13219155pfq.50.1605868442282; Fri, 20 Nov 2020 02:34:02 -0800 (PST) MIME-Version: 1.0 References: <13156732-5189-af54-3d28-e2ee2a8568e0@oss.nttdata.com> <069fb3bc-3907-4d85-19fd-e4b2d2f004b4@oss.nttdata.com> <1ae40b1b-2f42-4510-1514-40bb7d7255ab@oss.nttdata.com> In-Reply-To: <1ae40b1b-2f42-4510-1514-40bb7d7255ab@oss.nttdata.com> From: Bharath Rupireddy Date: Fri, 20 Nov 2020 16:03:50 +0530 Message-ID: Subject: Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module To: Fujii Masao Cc: Craig Ringer , PostgreSQL-development , Robert Haas Content-Type: multipart/alternative; boundary="0000000000000ef67c05b48762d8" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000000ef67c05b48762d8 Content-Type: text/plain; charset="UTF-8" On Wed, Nov 18, 2020 at 5:52 PM Fujii Masao 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 --0000000000000ef67c05b48762d8 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Wed, Nov 18, 2020 at 5:52 PM Fujii Masao <masao.fujii@oss.nttdata.com> wr= ote:
>
> > handle_sigterm() and die() are similar except tha= t die() has extra
> > handling(below) for exiting immediately when= waiting for input/command
> > from the client.
> > =C2= =A0 =C2=A0 =C2=A0/*
> > =C2=A0 =C2=A0 =C2=A0 * If we're in sin= gle user mode, we want to quit immediately - we can't
> > =C2= =A0 =C2=A0 =C2=A0 * rely on latches as they wouldn't work when stdin/st= dout is a file.
> > =C2=A0 =C2=A0 =C2=A0 * Rather ugly, but it'= ;s unlikely to be worthwhile to invest much more
> > =C2=A0 =C2=A0= =C2=A0 * effort just for the benefit of single user mode.
> > =C2= =A0 =C2=A0 =C2=A0 */
> > =C2=A0 =C2=A0 =C2=A0if (DoingCommandRead = && whereToSendOutput !=3D DestRemote)
> > =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0ProcessInterrupts();
> >
> > Having t= his 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
> &= gt; being set to true only in normal backend PostgresMain(). If we use
&= gt; > 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 pr= ocessed
> > immediately. And also here are the bg worker process t= hat use die()
> > instead of their own custom handlers: parallel w= orkers, autoprewarm
> > worker, autovacuum worker, logical replica= tion 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.<= br>>

I think the commit 2505ce0 that altered the comment has some= thing: handle_sigterm() is stripped down to remove if (DoingCommandRead &am= p;& whereToSendOutput !=3D DestRemote) part from die() since test_shm_m= q bg worker or for that matter any bg worker do not have an equivalent of t= he regular backend's command-read loop.

--- a/src/test/modules/t= est_shm_mq/worker.c
+++ b/src/test/modules/test_shm_mq/worker.c
@@ -6= 0,13 +60,9 @@ test_shm_mq_main(Datum main_arg)
=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0*
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* We want CHECK_FOR_INTERR= UPTS() to kill off this worker process just as
=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0* it would a normal user backend.=C2=A0 To make that happen, we e= stablish a
- =C2=A0 =C2=A0 =C2=A0 =C2=A0* signal handler that is a strip= ped-down version of die(). =C2=A0We don&= #39;t have
- =C2=A0 =C2=A0 =C2=A0 =C2=A0* any equivalent of the backend&= #39;s command-read loop, where interrupts can
- =C2=A0 =C2=A0 =C2=A0 =C2= =A0* be processed immediately, so make sure ImmediateInterruptOK is = turned
- =C2=A0 =C2=A0 =C2=A0 =C2=A0* off.
+ =C2=A0 =C2=A0 =C2=A0 =C2= =A0* signal handler that is a stripped-down version of die().
=C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0*/
=C2=A0 =C2=A0 =C2=A0 =C2=A0 pqsignal(SIGTERM,= handle_sigterm);
- =C2=A0 =C2=A0 =C2=A0 ImmediateInterruptOK =3D false;=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 BackgroundWorkerUnblockSignals();

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

Thanks.

With Regards,
Bharath Rupireddy.
Enterpr= iseDB: http://www.enterprisedb.com<= /a>
--0000000000000ef67c05b48762d8--