Re: Add Information during standby recovery conflicts

On Mon, Nov 30, 2020 at 3:46 PM Drouvot, Bertrand <bdrouvot(at)amazon(dot)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(at)amazon(dot)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/