Re: Invalid primary_slot_name triggers warnings in all processes on reload

On Thu, Oct 9, 2025 at 2:06 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> Are you still planning to work on it?

Yes, thanks for the ping!
Attached is the updated version of the patch.

> Please note that since you're using already translated strings as
> arguments, you must use errmsg_internal() and errhint_internal(), to
> avoid doubly-translating these messages.

I've updated the patch to use errmsg_internal() and errhint_internal().
However, for GUCs, GUC_check_errdetail() and GUC_check_errhint() are
still used - and since they also translate their input, we might need
something like GUC_check_errdetail_internal() and
GUC_check_errhint_internal() to avoid double translation. Thought?
I haven't added those functions in the attached patch yet.

This isn't directly related to this thread, but while updating the patch,
I noticed that the call_xxx_check_hook() functions in guc.c use errhint()
instead of errhint_internal(). That might also cause double translation.
Interestingly, for log and detail messages they already use errmsg_internal()
and errdetail_internal(). Only the HINT messages still use the non-internal
version. Should we switch to use errhint_internal() as well?


> I am asking because it is better
> to fix this before merging the change for another somewhat related
> bug-fix patch being discussed in thread [1].

+1

> > BTW, about validating primary_slot_name in the check hook: I'm not sure
> > it's really worthwhile. Even without this validation, an invalid slot name
> > will raise an error anyway, and this validation can't catch all invalid cases.
> > So its value seems limited. If the check hook doesn't need to do
> > this validation, then ReplicationSlotValidateName() doesn't need to be
> > updated for that purpose.
> >
>
> Right. I think we can consider this separately just as a HEAD patch.

+1

Regards,

--
Fujii Masao