Re: Invalid primary_slot_name triggers warnings in all processes on reload

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.

> +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.

> +    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.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"El hombre nunca sabe de lo que es capaz hasta que lo intenta" (C. Dickens)