Re: Add Information during standby recovery conflicts

On Mon, Nov 16, 2020 at 4:55 PM Drouvot, Bertrand <bdrouvot(at)amazon(dot)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?

---
@@ -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.

Regards,

--
Masahiko Sawada
EnterpriseDB: https://www.enterprisedb.com/