Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

On 2020/12/16 16:24, Kyotaro Horiguchi wrote:
> At Tue, 15 Dec 2020 23:10:28 +0900, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)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.

Agreed. Attached is the patch that reverts those patches and
adds the comments about why both procLatch and recoveryWakeupLatch
are necessary.

Regards,

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