Re: Add Information during standby recovery conflicts

On Thu, 15 Oct 2020 at 12:13, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> wrote:
>
> At Wed, 14 Oct 2020 17:39:20 +0900, Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote in
> > On Wed, 14 Oct 2020 at 07:44, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> > >
> > > On 2020-Oct-14, Masahiko Sawada wrote:
> > >
> > > > I've attached the patch as an idea of fixing the above comments as
> > > > well as the comment from Alvaro. I can be applied on top of v4 patch.
> > >
> > > One note about the translation stuff. Currently you have _("...") where
> > > the string is produced, and then ereport(.., errmsg("%s", str) where it
> > > is used. Both those things will attempt to translate the string, which
> > > isn't great. It is better if we only translate once. You have two
> > > options to fix this: one is to change _() to gettext_noop() (which marks
> > > the string for translation so that it appears in the message catalog,
> > > but it does not return the translation -- it returns the original, and
> > > then errmsg() translates at run time). The other is to change errmsg()
> > > to errmsg_internal() .. so the function returns the translated message
> > > and errmsg_internal() doesn't apply a translation.
> > >
> > > I prefer the first option, because if we ever include a server feature
> > > to log both the non-translated message alongside the translated one, we
> > > will already have both in hand.
> >
> > Thanks, I didn't know that. So perhaps ATWrongRelkindError() has the
> > same translation problem? It uses _() when producing the message but
> > also uses errmsg().
> >
> > I've attached the patch changed accordingly. I also fixed some bugs
> > around recovery conflicts on locks and changed the code so that the
> > log shows pids instead of virtual transaction ids since pids are much
> > easy to use for the users.
>
> You're misunderstanding.

Thank you! That's helpful for me.

> 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?

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).

Regards,

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