Horton

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Fujii Masao <masao.fujii@oss.nttdata.com>

View thread View on postgr.es
From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2020-11-12T04:36:07Z
Lists: pgsql-hackers 

On 2020/11/10 21:30, Bharath Rupireddy wrote:
> On Tue, Nov 10, 2020 at 3:04 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>
>>>> The main reason for having SetLatch() in
>>>> SignalHandlerForConfigReload() is to wake up the calling process if
>>>> waiting in WaitLatchOrSocket() or WaitLatch() and reload the new
>>>> config file and use the reloaded config variables. Maybe we should
>>>> give a thought on the scenarios in which the walreceiver process
>>>> waits, and what happens in case the latch is set when SIGHUP is
>>>> received.
>>>
>>> The difference is whether the config file is processed at the next
>>> wakeup (by force-reply-request or SIGTERM) of walreceiver or
>>> immediately. If server-reload happened frequently, say, several times
>>> per second(?), we should consider to reduce the useless reloading, but
>>> actually that's not the case.
>>
>> So, attached is the patch that makes walreceiver use both standard
>> SIGTERM and SIGHUP handlers. Currently I've not found any actual
>> issues by making walreceiver use standard SIGHUP handler, yet.
>>
> 
> I think it makes sense to replace WalRcv->latch with
> MyProc->procLatch(as they point to the same latch) in the functions
> that are called in the walreceiver process. However, we are using
> walrcv->latch with spinlock, say in WalRcvForceReply() and
> RequestXLogStreaming() both are called from the startup process. See
> commit 45f9d08684, that made the access to the walreceiver's
> latch(WalRcv->latch) by the other process(startup) spinlock protected
> 
> And looks like, in general it's a standard practice to set latch to
> wake up the process if waiting in case a SIGHUP signal is received and
> reload the relevant config variables.
> 
> Going by the above analysis, the v3 patch looks good to me.

Thanks for the analysis! I pushed the patch.

Regards,

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

Commits

  1. Revert "Get rid of the dedicated latch for signaling the startup process".

  2. Improve log message about termination of background workers.

  3. Use standard SIGTERM signal handler die() in test_shm_mq worker.

  4. Use standard SIGHUP and SIGTERM signal handlers in worker_spi.

  5. Use standard SIGHUP and SIGTERM handlers in walreceiver.

  6. pg_prewarm: make autoprewarm leader use standard SIGHUP and SIGTERM handlers.

  7. Get rid of the dedicated latch for signaling the startup process.

  8. Use standard SIGHUP handler in syslogger.

  9. Use PostgresSigHupHandler in more places.

  10. Fix race condition with unprotected use of a latch pointer variable.

  11. Remove remnants of ImmediateInterruptOK handling.

  12. Introduce Streaming Replication.