Re: Invalid primary_slot_name triggers warnings in all processes on reload - Mailing list pgsql-hackers

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.

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.


> > +error:
> > +     if (elevel == 0)
> > +     {
> > +             GUC_check_errdetail("%s", err_msg);
> > +             if (err_hint != NULL)
> > +                     GUC_check_errhint("%s", err_hint);
> > +     }
> > +     else
> > +             ereport(elevel,
> > +                             (errcode(err_code),
> > +                              errmsg("%s", err_msg),
> > +                              (err_hint != NULL) ? errhint("%s", err_hint) : 0));
>
> 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.

Right, thanks for pointing that out.


> > +     pfree(err_msg);
> > +     if (err_hint != NULL)
> > +             pfree(err_hint);
> > +
> > +     return false;
>
> Not sure how valuable pfree'ing these really is ... I suspect this is
> only called in contexts that aren't sensitive to a few bytes of
> transient leakage.

I initially thought the same since the short-lived config file processing
context is used when handling the config file.
But when ReplicationSlotValidateName() is called outside the GUC check hook,
seems allocations can end up in TopMemoryContext. Even though the memory use
is small, it's safer to call pfree(), and the overhead is negligible.
So I didn't see a strong reason to skip it....

Regards,

--
Fujii Masao