Re: Add Information during standby recovery conflicts

On Mon, Nov 9, 2020 at 2:49 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
>
> Hi,
>
> On 11/6/20 3:21 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 Fri, Oct 30, 2020 at 6:02 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
> >> Hi,
> >>
> >> On 10/30/20 4:25 AM, Fujii Masao 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 2020/10/30 10:29, Masahiko Sawada wrote:
> >>>> ,
> >>>>
> >>>> On Thu, 29 Oct 2020 at 00:16, Fujii Masao
> >>>> <masao.fujii@oss.nttdata.com> wrote:
> >>>>> I read v8 patch. Here are review comments.
> >>>> Thank you for your review.
> >>>>
> > Thank you for updating the patch!
> >
> > Looking at the latest version patch
> > (v8-0002-Log-the-standby-recovery-conflict-waits.patch), I think it
> > doesn't address some comments from Fujii-san.
> >
> >>>>> When recovery conflict with buffer pin happens, log message is output
> >>>>> every deadlock_timeout. Is this intentional behavior? If yes, IMO
> >>>>> that's
> >>>>> not good because lots of messages can be output.
> >>>> Agreed.
> > I think the latest patch doesn't fix the above comment. Log message
> > for recovery conflict on buffer pin is logged every deadlock_timeout.
> >
> >>>> if we were to log the recovery conflict only once in bufferpin
> >>>> conflict case, we would log it multiple times only in lock conflict
> >>>> case. So I guess it's better to do that in all conflict cases.
> >>> Yes, I agree that this behavior basically should be consistent between
> >>> all cases.
> > The latest patch seems not to address this comment as well.
>
> Oh, I missed those ones, thanks for the feedback.
>
> New version attached, so that recovery conflict will be logged only once
> also for buffer pin and lock cases.

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.

Regards,

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