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)