Horton

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Kyotaro Horiguchi <horikyota.ntt@gmail.com>

View thread View on postgr.es
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
To: masao.fujii@oss.nttdata.com
Cc: bharath.rupireddyforpostgres@gmail.com, pgsql-hackers@postgresql.org
Date: 2020-12-16T07:24:59Z
Lists: pgsql-hackers 
At Tue, 15 Dec 2020 23:10:28 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> > I pushed the following two patches.
> > - v1-use-standard-SIGHUP-hanlder-in-syslogger-process.patch
> > - v1-use-MyLatch-and-standard-SIGHUP-handler-in-startup-process.patch
> 
> As I told in other thread [1], I'm thinking to revert this patch
> because this change could have bad side-effect on the startup
> process waiting for recovery conflict.
> 
> Before applying the patch, the latch that the startup process
> used to wait for recovery conflict was different from the latch
> that SIGHUP signal handler or walreceiver process, etc set to
> wake the startup process up. So SIGHUP or walreceiver didn't
> wake the startup process waiting for recovery conflict up unnecessary.
>
> But the patch got rid of the dedicated latch for signaling
> the startup process. This change forced us to use the same latch
> to make the startup process wait or wake up. Which caused SIGHUP
> signal handler or walreceiver proces to wake the startup process
> waiting on the latch for recovery conflict up unnecessarily
> frequently.
> 
> While waiting for recovery conflict on buffer pin, deadlock needs
> to be checked at least every deadlock_timeout. But that frequent
> wakeups could prevent the deadlock timer from being triggered and
> could delay that deadlock checks.

I thought that spurious wakeups don't harm. But actually
ResolveRecoveryConflictWithBufferPin doesn't consider spurious
wakeups.  Only the timer woke up ResolveRecoveryConflictWithBufferPin
before the patch comes. Currently SIGHUP and XLogFlush() (on
walreceiver) also wake up startup process.

For a moment I thought that ResolveRecoveryConflictWithBufferPin
should wake up at shutdown time by the old recovery latch but it's not
the case since it wakes up after all blockers go away.  It seems to me
simpler to revert the patches than making the function properly handle
spurious wakeups.

> Therefore, I'm thinking to revert the commit ac22929a26 and
> 113d3591b8.
> 
> [1]
> https://www.postgresql.org/message-id/a802f1c0-58d9-bd3f-bc3e-bdad54726855@oss.nttdata.com

As the result, I agree to revert them. But I think we need to add a
comment for the reason we don't use MyLatch for recovery-wakeup after
reverting them.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

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.