Re: Add Information during standby recovery conflicts

On Tue, 20 Oct 2020 at 22:02, Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
>
> Hi,
>
> On 10/15/20 9:15 AM, Masahiko Sawada wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you
canconfirm the sender and know the content is safe. 
> >
> >
> >
> > On Thu, 15 Oct 2020 at 14:52, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> >> At Thu, 15 Oct 2020 14:28:57 +0900, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote in
> >>>> ereport(..(errmsg("%s", _("hogehoge")))) results in
> >>>> fprintf((translated("%s")), translate("hogehoge")).
> >>>>
> >>>> So your change (errmsg("%s", gettext_noop("hogehoge")) results in
> >>>>
> >>>> fprintf((translated("%s")), DONT_translate("hogehoge")).
> >>>>
> >>>> which leads to a translation problem.
> >>>>
> >>>> (errmsg(gettext_noop("hogehoge"))
> >>> This seems equivalent to (errmsg("hogehoge")), right?
> >> Yes and no.  However eventually the two works the same way,
> >> "(errmsg(gettext_noop("hogehoge"))" is a shorthand of
> >>
> >> 1: char *msg = gettext_noop("hogehoge");
> >> ...
> >> 2: .. (errmsg(msg));
> >>
> >> That is, the line 1 only registers a message id "hogehoge" and doesn't
> >> translate. The line 2 tries to translate the content of msg and it
> >> finds the translation for the message id "hogehoge".
> > Understood.
> >
> >>> I think I could understand translation stuff. Given we only report the
> >>> const string returned from get_recovery_conflict_desc() without
> >>> placeholders, the patch needs to use errmsg_internal() instead while
> >>> not changing _() part. (errmsg(get_recovery_conflict_desc())) is not
> >>> good (warned by -Wformat-security).
> >> Ah, right. we get a complain if no value parameters added. We can
> >> silence it by adding a dummy parameter to errmsg, but I'm not sure
> >> which is preferable.
> > Okay, I'm going to use errmsg_internal() for now until a better idea comes.
> >
> > I've attached the updated patch that fixed the translation part.
>
> Thanks for reviewing and helping on this patch!
>
> The patch tester bot is currently failing due to:
>
> "proc.c:1290:5: error: ‘standbyWaitStart’ may be used uninitialized in
> this function [-Werror=maybe-uninitialized]"
>
> I've attached a new version with the minor change to fix it.
>

Thank you for updating the patch!

I've looked at the patch and revised a bit the formatting etc.

After some thoughts, I think it might be better to report the waiting
time as well. it would help users and there is no particular reason
for logging the report only once. It also helps make the patch clean
by reducing the variables such as recovery_conflict_logged. I’ve
implemented it in the v8 patch. The log message is now like:

 LOG:  recovery is still waiting recovery conflict on lock after 1062.601 ms
 DETAIL:  Conflicting processes: 35116, 35115, 35114.
 CONTEXT:  WAL redo at 0/3000028 for Standby/LOCK: xid 510 db 13185 rel 16384

LOG:  recovery is still waiting recovery conflict on lock after 2065.682 ms
DETAIL:  Conflicting processes: 35115, 35114.
CONTEXT:  WAL redo at 0/3000028 for Standby/LOCK: xid 510 db 13185 rel 16384

LOG:  recovery is still waiting recovery conflict on lock after 3087.926 ms
DETAIL:  Conflicting process: 35114.
CONTEXT:  WAL redo at 0/3000028 for Standby/LOCK: xid 510 db 13185 rel 16384

What do you think?

Regards,

--
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services