Re: Add Information during standby recovery conflicts

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?

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