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)