Re: Add Information during standby recovery conflicts

On 11/17/20 2:09 AM, Masahiko Sawada wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Mon, Nov 16, 2020 at 4:55 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
>> Hi,
>>
>> On 11/16/20 6:44 AM, Masahiko Sawada wrote:
>>> Thank you for updating the patch.
>>>
>>> Here are review comments.
>>>
>>> +           if (report_waiting && (!logged_recovery_conflict ||
>>> new_status == NULL))
>>> +               ts = GetCurrentTimestamp();
>>>
>>> The condition will always be true if log_recovery_conflict_wait is
>>> false and report_waiting is true, leading to unnecessary calling of
>>> GetCurrentTimestamp().
>>>
>>> ---
>>> +   <para>
>>> +    You can control whether a log message is produced when the startup process
>>> +    is waiting longer than <varname>deadlock_timeout</varname> for recovery
>>> +    conflicts. This is controled by the <xref
>>> linkend="guc-log-recovery-conflict-waits"/>
>>> +    parameter.
>>> +   </para>
>>>
>>> s/controled/controlled/
>>>
>>> ---
>>>       if (report_waiting)
>>>           waitStart = GetCurrentTimestamp();
>>>
>>> Similarly, we have the above code but we don't need to call
>>> GetCurrentTimestamp() if update_process_title is false, even if
>>> report_waiting is true.
>>>
>>> I've attached the patch that fixes the above comments. It can be
>>> applied on top of your v8 patch.
>> Thanks for the review and the associated fixes!
>>
>> I've attached a new version that contains your fixes.
>>
> Thank you for updating the patch.
>
> I have other comments:
>
> +   <para>
> +    You can control whether a log message is produced when the startup process
> +    is waiting longer than <varname>deadlock_timeout</varname> for recovery
> +    conflicts. This is controlled by the
> +    <xref linkend="guc-log-recovery-conflict-waits"/> parameter.
> +   </para>
>
> It would be better to use 'WAL replay' instead of 'the startup
> process' for consistency with circumjacent descriptions. What do you
> think?

Agree that the wording is more appropriate.

>
> ---
> @@ -1260,6 +1262,8 @@ ProcSleep(LOCALLOCK *locallock, LockMethod
> lockMethodTable)
>    else
>    enable_timeout_after(DEADLOCK_TIMEOUT, DeadlockTimeout);
>    }
> + else
> +   standbyWaitStart = GetCurrentTimestamp();
>
> I think we can add a check of log_recovery_conflict_waits to avoid
> unnecessary calling of GetCurrentTimestamp().
>
> I've attached the updated version patch including the above comments
> as well as adding some assertions. Please review it.
>
That looks all good to me.

Thanks a lot for your precious help!

Bertrand