Re: Add Information during standby recovery conflicts

On Mon, Nov 30, 2020 at 3:46 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
>
> Hi,
>
> On 11/30/20 4:41 AM, Masahiko Sawada wrote:
> > On Sun, Nov 29, 2020 at 3:47 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
> >> Hi Alvaro,
> >>
> >> On 11/28/20 6:36 PM, Alvaro Herrera wrote:
> >>> Hi Bertrand,
> >>>
> >>> On 2020-Nov-28, Drouvot, Bertrand wrote:
> >>>
> >>>> +             if (nprocs > 0)
> >>>> +             {
> >>>> +                     ereport(LOG,
> >>>> +                                     (errmsg("recovery still waiting after %ld.%03d ms: %s",
> >>>> +                                                     msecs, usecs, _(get_recovery_conflict_desc(reason))),
> >>>> +                                      (errdetail_log_plural("Conflicting process: %s.",
> >>>> +                                                                                "Conflicting processes: %s.",
> >>>> +                                                                                nprocs, buf.data))));
> >>>> +             }
> >>>> +             else
> >>>> +             {
> >>>> +                     ereport(LOG,
> >>>> +                                     (errmsg("recovery still waiting after %ld.%03d ms: %s",
> >>>> +                                                     msecs, usecs, _(get_recovery_conflict_desc(reason)))));
> >>>> +             }
> >>>> +
> >>>> +             pfree(buf.data);
> >>>> +     }
> >>>> +     else
> >>>> +             ereport(LOG,
> >>>> +                             (errmsg("recovery still waiting after %ld.%03d ms: %s",
> >>>> +                                             msecs, usecs, _(get_recovery_conflict_desc(reason)))));
> >>>> +}
> >>> Another trivial stylistic point is that you can collapse all these
> >>> ereport calls into one, with something like
> >>>
> >>>     ereport(LOG,
> >>>             errmsg("recovery still waiting after ...", opts),
> >>>             waitlist != NULL ? errdetail_log_plural("foo bar baz", opts) : 0);
> >>>
> >>> where the "waitlist" has been constructed beforehand, or is set to NULL
> >>> if there's no process list.
> >> Nice!
> >>
> >>>> +     switch (reason)
> >>>> +     {
> >>>> +             case PROCSIG_RECOVERY_CONFLICT_BUFFERPIN:
> >>>> +                     reasonDesc = gettext_noop("for recovery conflict on buffer pin");
> >>>> +                     break;
> >>> Pure bikeshedding after discussing this with my pillow: I think I'd get
> >>> rid of the initial "for" in these messages.
> >> both comments implemented in the new attached version.
> >>
> > Thank you for updating the patch!
> >
> > +       /* Also, set deadlock timeout for logging purpose if necessary */
> > +       if (log_recovery_conflict_waits && !need_log)
> > +       {
> > +           timeouts[cnt].id = STANDBY_TIMEOUT;
> > +           timeouts[cnt].type = TMPARAM_AFTER;
> > +           timeouts[cnt].delay_ms = DeadlockTimeout;
> > +           cnt++;
> > +       }
> >
> > You changed to 'need_log' but this condition seems not correct. I
> > think we need to set this timeout when both log_recovery_conflict and
> > need_log is true.
>
> Nice catch!
>
> In fact it behaves correctly, that's jut the 'need_log' name that is
> miss leading: I renamed it to 'already_logged' in the new attached version.
>

Thanks! I'd prefer 'need_log' because we can check it using the
affirmative form in that condition, which would make the code more
readable a bit. But I'd like to leave it to committers. I've marked
this patch as "Ready for Committer".

Regards,

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