Re: Deadlock between backend and recovery may not be detected

On 2020/12/22 10:25, Masahiko Sawada wrote:
> On Fri, Dec 18, 2020 at 6:36 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>
>>
>>
>> On 2020/12/17 2:15, Fujii Masao wrote:
>>>
>>>
>>> On 2020/12/16 23:28, Drouvot, Bertrand wrote:
>>>> Hi,
>>>>
>>>> On 12/16/20 2:36 PM, Victor Yegorov wrote:
>>>>>
>>>>> *CAUTION*: This email originated from outside of the organization. Do not click links or open attachments unless
youcan confirm the sender and know the content is safe.
 
>>>>>
>>>>>
>>>>> ср, 16 дек. 2020 г. в 13:49, Fujii Masao <masao.fujii@oss.nttdata.com <mailto:masao.fujii@oss.nttdata.com>>:
>>>>>
>>>>>      After doing this procedure, you can see the startup process and backend
>>>>>      wait for the table lock each other, i.e., deadlock. But this deadlock remains
>>>>>      even after deadlock_timeout passes.
>>>>>
>>>>>      This seems a bug to me.
>>>>>
>>>> +1
>>>>
>>>>>
>>>>>      > * Deadlocks involving the Startup process and an ordinary backend process
>>>>>      > * will be detected by the deadlock detector within the ordinary backend.
>>>>>
>>>>>      The cause of this issue seems that ResolveRecoveryConflictWithLock() that
>>>>>      the startup process calls when recovery conflict on lock happens doesn't
>>>>>      take care of deadlock case at all. You can see this fact by reading the above
>>>>>      source code comment for ResolveRecoveryConflictWithLock().
>>>>>
>>>>>      To fix this issue, I think that we should enable STANDBY_DEADLOCK_TIMEOUT
>>>>>      timer in ResolveRecoveryConflictWithLock() so that the startup process can
>>>>>      send PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK signal to the backend.
>>>>>      Then if PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK signal arrives,
>>>>>      the backend should check whether the deadlock actually happens or not.
>>>>>      Attached is the POC patch implimenting this.
>>>>>
>>>> good catch!
>>>>
>>>> I don't see any obvious reasons why the STANDBY_DEADLOCK_TIMEOUT shouldn't be set in
ResolveRecoveryConflictWithLock()too (it is already set in ResolveRecoveryConflictWithBufferPin()).
 
>>>>
>>>> So + 1 to consider this as a bug and for the way the patch proposes to fix it.
>>>
>>> Thanks Victor and Bertrand for agreeing!
>>> Attached is the updated version of the patch.
>>
>> Attached is v3 of the patch. Could you review this version?
>>
>> While the startup process is waiting for recovery conflict on buffer pin,
>> it repeats sending the request for deadlock check to all the backends
>> every deadlock_timeout. This may increase the workload in the startup
>> process and backends, but since this is the original behavior, the patch
>> doesn't change that. Also in practice this may not be so harmful because
>> the period that the buffer is kept pinned is basically not so long.
>>
> 
> @@ -529,6 +603,26 @@ ResolveRecoveryConflictWithBufferPin(void)
>       */
>      ProcWaitForSignal(PG_WAIT_BUFFER_PIN);
> 
> +   if (got_standby_deadlock_timeout)
> +   {
> +       /*
> +        * Send out a request for hot-standby backends to check themselves for
> +        * deadlocks.
> +        *
> +        * XXX The subsequent ResolveRecoveryConflictWithBufferPin() will wait
> +        * to be signaled by UnpinBuffer() again and send a request for
> +        * deadlocks check if deadlock_timeout happens. This causes the
> +        * request to continue to be sent every deadlock_timeout until the
> +        * buffer is unpinned or ltime is reached. This would increase the
> +        * workload in the startup process and backends. In practice it may
> +        * not be so harmful because the period that the buffer is kept pinned
> +        * is basically no so long. But we should fix this?
> +        */
> +       SendRecoveryConflictWithBufferPin(
> +
> PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK);
> +       got_standby_deadlock_timeout = false;
> +   }
> +
> 
> Since SendRecoveryConflictWithBufferPin() sends the signal to all
> backends every backend who is waiting on a lock at ProcSleep() and not
> holding a buffer pin blocking the startup process will end up doing a
> deadlock check, which seems expensive. What is worse is that the
> deadlock will not be detected because the deadlock involving a buffer
> pin isn't detected by CheckDeadLock(). I thought we can replace
> PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK with
> PROCSIG_RECOVERY_CONFLICT_BUFFERPIN but it’s not good because the
> backend who has a buffer pin blocking the startup process and not
> waiting on a lock is also canceled after deadlock_timeout. We can have
> the backend return from RecoveryConflictInterrupt() when it received
> PROCSIG_RECOVERY_CONFLICT_BUFFERPIN and is not waiting on any lock,
> but it’s also not good because we cannot cancel the backend after
> max_standby_streaming_delay that has a buffer pin blocking the startup
> process. So I wonder if we can have a new signal. When the backend
> received this signal it returns from RecoveryConflictInterrupt()
> without deadlock check either if it’s not waiting on any lock or if it
> doesn’t have a buffer pin blocking the startup process. Otherwise it's
> cancelled.

Thanks for pointing out that issue! Using new signal is an idea. Another idea
is to make a backend skip check the deadlock if GetStartupBufferPinWaitBufId()
returns -1, i.e., the startup process is not waiting for buffer pin. So,
what I'm thinkins is;

In RecoveryConflictInterrupt(), when a backend receive
PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK,

1. If a backend isn't waiting for a lock, it does nothing .
2. If a backend is waiting for a lock and also holding a buffer pin that
    delays recovery, it may be canceled.
3. If a backend is waiting for a lock and the startup process is not waiting
    for buffer pin (i.e., the startup process is also waiting for a lock),
    it checks for the deadlocks.
4. If a backend is waiting for a lock and isn't holding a buffer pin that
    delays recovery though the startup process is waiting for buffer pin,
    it does nothing.

Thought?

Regards,

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