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

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 found other examples similar to this -- see the
error_view_not_updatable() function in rewriteHandler.c which does:
ereport(ERROR,
...
detail ? errdetail_internal("%s", _(detail)) : 0,
...

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