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
canconfirm 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
