Re: Invalid primary_slot_name triggers warnings in all processes on reload

On Wed, Sep 24, 2025 at 11:57 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Wed, Sep 24, 2025 at 12:11 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> >
> > Hello,
> >
> >
> > >  /*
> > > - * Check whether the passed slot name is valid and report errors at elevel.
> > > + * Check whether the passed slot name is valid and report errors.
> > > + *
> > > + * When called from a GUC check hook, elevel must be 0. In that case,
> > > + * errors are reported as additional details or hints using
> > > + * GUC_check_errdetail() and GUC_check_errhint(). Otherwise, elevel
> > > + * must be nonzero, and errors are reported at that log level with ereport().
> >
> > I find this an odd calling convention; I don't think we use it anywhere
> > else and I wonder if we shouldn't split this up in two functions calling
> > a third one that returns the string messages, to avoid the oddity.  I'd
> > do something like
> >
> > bool
> > ReplicationSlotValidateName(const char *name, bool allow_reserved_name,
> >                             char **err_msg, char **err_hint)
> >
> > and then if this returns false, err_msg and err_hint have been populated
> > and can be used to throw the error or added as GUC_check_* arguments, or
> > just ignored.
>
> Yeah, that's an option. If we go with it, we'd probably need to return
> the error code as well. Since it looks better, I'll consider updating
> the patch accordingly.
>

Are you still planning to work on it? 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].

> 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] - https://www.postgresql.org/message-id/CAA5-nLB0JhVO-ykpfguxyGkoAk1tECi5xMTqAoVJ6spnDiQzaw%40mail.gmail.com

--
With Regards,
Amit Kapila.