Re: Introduce XID age and inactive timeout based replication slot invalidation

On Mon, Feb 3, 2025 at 4:04 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Feb 3, 2025 at 9:04 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > On Fri, Jan 31, 2025 at 8:02 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Fri, Jan 31, 2025 at 10:40 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > > >
> > > > ======
> > > > src/backend/replication/slot.c
> > > >
> > > > ReportSlotInvalidation:
> > > >
> > > > 1.
> > > > +
> > > > + case RS_INVAL_IDLE_TIMEOUT:
> > > > + Assert(inactive_since > 0);
> > > > + /* translator: second %s is a GUC variable name */
> > > > + appendStringInfo(&err_detail,
> > > > + _("The slot has remained idle since %s, which is longer than the
> > > > configured \"%s\" duration."),
> > > > + timestamptz_to_str(inactive_since),
> > > > + "idle_replication_slot_timeout");
> > > > + break;
> > > > +
> > > >
> > > > errdetail:
> > > >
> > > > I guess it is no fault of this patch because I see you've only copied
> > > > nearby code, but AFAICT this function is still having an each-way bet
> > > > by using a mixture of _() macro which is for strings intended be
> > > > translated, but then only using them in errdetail_internal() which is
> > > > for strings that are NOT intended to be translated. Isn't it
> > > > contradictory? Why don't we use errdetail() here?
> > > >
> > >
> > > Your question is valid and I don't have an answer. I encourage you to
> > > start a new thread to clarify this.
> > >
> >
> > I think this was a false alarm.
> >
> > After studying this more deeply, I've changed my mind and now think
> > the code is OK as-is.
> >
> > AFAICT errdetail_internal is used when not wanting to translate the
> > *fmt* string passed to it (see EVALUATE_MESSAGE in elog.c). Now, here
> > the format string is just "%s" so it's fine to not translate that.
> > Meanwhile, the string value being substituted to the "%s" was already
> > translated because of the _(x) macro aka gettext(x).
> >
>
> I didn't get your point about " the "%s" was already translated
> because of ...". If we don't want to translate the message then why
> add '_(' to it in the first place?
>

I think this is same point where I was fooling myself yesterday. In
fact we do want to translate the message seen by the user.

errdetail_internal really means don't translate the ***format
string***. In our case "%s" is not the message at all -- it is just
the a *format string* so translating "%s" is kind of meaningless.

e.g. Normally....

errdetail("translate me") <-- This would translate the fmt string but
here the fmt is also the message; i.e. it will do gettext("translate
me") internally.

errdetail_internal("translate me") <-- This won't translate anything;
you will have the raw fmt string "translate me"

~~

But since ReportSlotInvalidation is building the message on the fly
there is no single report so it is a bit different....

errdetail("%s", "translate me") <-- this would just use gettext("%s")
which is kind of useless. And the "translate me" is just a raw string
and won't be translated.

errdetail_internal("%s", "translate me") <-- this won't translate
anything; the fmt string and the "translate me" are just raw strings

errdetail_internal("%s", _("translate me")) <-- This won't translate
the fmt string, but to translate %s is useless anyway. OTOH, the _()
macro means it will do gettext("translate me") so the "translate me"
string will get translated before it is substituted. This is
effectively what the ReportSlotInvalidation code is doing.

======
Kind Regards,
Peter Smith.
Fujitsu Australia