MIME-Version: 1.0
References: <20201105.121206.819769369617763972.horikyota.ntt@gmail.com>
 <bc313fdc-2beb-52d1-b8a7-ef1e4cfb92bf@oss.nttdata.com>
 <CALj2ACWz1R+gRt2zJCSsWZeCgQhnM6-E-4XFotqqUwbyYk7ZEA@mail.gmail.com>
 <20201110.161740.1341818036035711165.horikyota.ntt@gmail.com>
 <eeda13ff-9ed4-747b-b47b-a7983e2fab40@oss.nttdata.com>
In-Reply-To: <eeda13ff-9ed4-747b-b47b-a7983e2fab40@oss.nttdata.com>
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 10 Nov 2020 18:00:07 +0530
Message-ID: 
 <CALj2ACXPEW6wpx+-s5dqcDV2Y+G0U-wu79ZHr7mPRF8G_YwB-Q@mail.gmail.com>
Subject: Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com