Subject: Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Craig Ringer <craig.ringer@enterprisedb.com>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>,
 Robert Haas <robert.haas@enterprisedb.com>
References: 
 <CALj2ACXPorUqePswDtOeM_s82v9RW32E1fYmOPZ5NuE+TWKj_A@mail.gmail.com>
 <13156732-5189-af54-3d28-e2ee2a8568e0@oss.nttdata.com>
 <CALj2ACV9NrMDT5Ndpm5STUYRBfKQJoPOgA5a27a1XRc4h8wVYw@mail.gmail.com>
 <069fb3bc-3907-4d85-19fd-e4b2d2f004b4@oss.nttdata.com>
 <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>
 <1ae40b1b-2f42-4510-1514-40bb7d7255ab@oss.nttdata.com>
 <CALj2ACUcBA2EK8Ey=Y+ugaRge0Ws2Fszwn-xhPsxt-FbzNiboQ@mail.gmail.com>
 <3f292fbb-f155-9a01-7cb2-7ccc9007ab3f@oss.nttdata.com>
 <CALj2ACVh2m4o+8T-Y9oXN22RUgGBAqcKcQdyA2+fgt4faeoGHQ@mail.gmail.com>
 <CALj2ACX4rQRHuczPnkr6Ki+PxdKR33aj9LYRFFO3t8Xgx=8eVw@mail.gmail.com>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
Message-ID: <9e3f88cd-0cc4-ac9e-4102-505ac8213fc9@oss.nttdata.com>
Date: Fri, 27 Nov 2020 15:56:52 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:78.0)
 Gecko/20100101 Thunderbird/78.5.0
MIME-Version: 1.0
In-Reply-To: 
 <CALj2ACX4rQRHuczPnkr6Ki+PxdKR33aj9LYRFFO3t8Xgx=8eVw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 2020/11/27 12:13, Bharath Rupireddy wrote:
> On Wed, Nov 25, 2020 at 8:08 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com <mailto:bharath.rupireddyforpostgres@gmail.com>> wrote:
>  >
>  > > When I read the patch again, I found that, with the patch, the shutdown
>  > > of worker_spi causes to report the following FATAL message.
>  > >
>  > >      FATAL:  terminating connection due to administrator command
>  > >
>  > > Isn't this message confusing because it's not a connection? If so,
>  > > we need to update ProcessInterrupts() so that the proper message is
>  > > reported like other bgworkers do.
>  > >
>  >
>  > This is also true for all the bgworker that use the die() handler. How about doing it the way bgworker_die() does in ProcessInterrupts()? This would give meaningful information. Thoughts? If okay, I can make a separate patch.
>  >
> 
> Attaching the patch that improved the message for bg workers in ProcessInterrupts(). For instance, now it looks like *FATAL:  terminating background worker "worker_spi" due to administrator command* or *FATAL:  terminating background worker "parallel worker" due to administrator command *and so on for other bg workers.*
> *
> 
> Please review the patch.

Thanks for the patch! It looks good to me.


> 
> I'm also mentioning the 2 previous patches posted in [1]. One of the patch is for using die() instead of handle_sigterm() in test_shm_mq/worker.c and another is for replacing custom SIGTERM handler worker_spi_sigterm() with die() and custom SIGHUP handler worker_spi_sighup() with standard SignalHandlerForConfigReload()

Yeah, I pushed them. Thanks!

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION