Horton

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>

View thread View on postgr.es
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2020-10-05T16:18:19Z
Lists: pgsql-hackers 
On Mon, Oct 5, 2020 at 8:04 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
> On 2020/10/05 19:45, Bharath Rupireddy wrote:
> > Hi,
> >
> > Autoprewarm module is using it's own SIGHUP(apw_sigterm_handler, got_sigterm) and SIGTERM(apw_sighup_handler, got_sighup) handlers which are similar to standard signal handlers(except for a difference [1]). Isn't it good to remove them and  use standard SignalHandlerForConfigReload and SignalHandlerForShutdownRequest?
> >
> > Attaching the patch for the above changes.
> >
> > Looks like the commit[2] replaced custom handlers with standard handlers.
> >
> > Thoughts?
>
> +1
>
> The patch looks good to me.
>

Thanks.

>
> ISTM that we can also replace StartupProcSigHupHandler() in startup.c
> with SignalHandlerForConfigReload() by making the startup process use
> the general shared latch instead of its own one. POC patch attached.
> Thought?
>

I'm not quite sure whether it breaks something or not. I see that
WakeupRecovery() with XLogCtl->recoveryWakeupLatch latch from the
startup process is also being used in the walreceiver process. I may
be wrong, but have some concern if the behaviour is different in case
of EXEC_BACKEND and Windows?

Another concern is that we are always using
XLogCtl->recoveryWakeupLatch in shared mode, this makes sense as this
latch is also being used in walrecevier. But sometimes, MyLatch is
created in non-shared mode as well(see InitLatch(MyLatch)).

Others may have better thoughts though.

>
> Probably we can also replace sigHupHandler() in syslogger.c with
> SignalHandlerForConfigReload()? This would be separate patch, though.
>

+1 to replace sigHupHandler() with SignalHandlerForConfigReload() as
the latch and the functionality are pretty much the same.

WalReceiverMai(): I think we can also replace WalRcvShutdownHandler()
with SignalHandlerForShutdownRequest() because walrcv->latch point to
&MyProc->procLatch which in turn point to MyLatch.

Thoughts? If okay, we can combine these into a single patch. I will
post an updated patch soon.

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

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.