On Mon, Nov 16, 2020 at 4:55 PM Drouvot, Bertrand <bdrou...@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/
v9-Log-the-standby-recovery-conflict-waits.patch
Description: Binary data
