Re: Add Information during standby recovery conflicts

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.

The rest of the patch looks good to me.

Regards,

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