Subject: Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: bharath.rupireddyforpostgres@gmail.com, pgsql-hackers@postgresql.org
References: <ce414f11-1418-511c-c840-a3cdd7f2b232@oss.nttdata.com>
 <6b561c73-27ce-7079-b6e5-440e49653810@oss.nttdata.com>
 <d8c0c608-021b-3c73-fffd-3240829ee986@oss.nttdata.com>
 <20201216.162459.1543880840255001035.horikyota.ntt@gmail.com>
 <68571154-5bd5-dc00-1322-92bbf92de806@oss.nttdata.com>
Message-ID: <d0944bcd-17f7-41d2-c297-eda963a8879a@oss.nttdata.com>
Date: Thu, 17 Dec 2020 18:12:39 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:78.0)
 Gecko/20100101 Thunderbird/78.5.1
MIME-Version: 1.0
In-Reply-To: <68571154-5bd5-dc00-1322-92bbf92de806@oss.nttdata.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 2020/12/16 18:12, Fujii Masao wrote:
> 
> 
> On 2020/12/16 16:24, Kyotaro Horiguchi wrote:
>> 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.
> 
> Agreed. Attached is the patch that reverts those patches and
> adds the comments about why both procLatch and recoveryWakeupLatch
> are necessary.

Pushed. Thanks!

Regards,

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