On Fri, Oct 10, 2025 at 2:52 PM Fujii Masao <[email protected]> wrote: > > On Thu, Oct 9, 2025 at 2:26 PM Fujii Masao <[email protected]> wrote: > > > 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. > > > > I've updated the patch to use errmsg_internal() and errhint_internal(). > > However, for GUCs, GUC_check_errdetail() and GUC_check_errhint() are > > still used - and since they also translate their input, we might need > > something like GUC_check_errdetail_internal() and > > GUC_check_errhint_internal() to avoid double translation. Thought? > > I haven't added those functions in the attached patch yet. > > OK, I've implemented this and attached two patches. > > 0001 fixes the issue we've been discussing. It's mostly the same as > the previous version I posted.
No comments on the latest patches — maybe that’s a good sign of their quality? ;) Anyway, unless there are any objections, I plan to commit at least the 0001 patch and backpatch it to all supported branches. I've attached the patches for the back branches for reference. Regarding the backpatch: in v17 and earlier, since errhint_internal() doesn't exist, I used errhint() instead. That means the hint message might be translated twice, but I think that's minor and acceptable. Or do you think we should instead backpatch errhint_internal() to those older branches to avoid the double translation? Regards, -- Fujii Masao
From 77bf7e4d0aff6309037a5c97d1f47078193fdb6f Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Tue, 21 Oct 2025 14:20:02 +0900 Subject: [PATCH v6] Make invalid primary_slot_name follow standard GUC error reporting. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, if primary_slot_name was set to an invalid slot name and the configuration file was reloaded, both the postmaster and all other backend processes reported a WARNING. With many processes running, this could produce a flood of duplicate messages. The problem was that the GUC check hook for primary_slot_name reported errors at WARNING level via ereport(). This commit changes the check hook to use GUC_check_errdetail() and GUC_check_errhint() for error reporting. As with other GUC parameters, this causes non-postmaster processes to log the message at DEBUG3, so by default, only the postmaster's message appears in the log file. Backpatch to all supported versions. Author: Fujii Masao <[email protected]> Reviewed-by: Chao Li <[email protected]> Reviewed-by: Amit Kapila <[email protected]> Reviewed-by: Álvaro Herrera <[email protected]> Discussion: https://postgr.es/m/cahgqgwfud-cvthctfusbfkhbs6jj6kdaptdlwkvp2qjux6l...@mail.gmail.com Backpatch-through: 13 --- src/backend/access/transam/xlogrecovery.c | 13 ++++- src/backend/replication/slot.c | 59 +++++++++++++++++------ src/include/replication/slot.h | 2 + 3 files changed, 58 insertions(+), 16 deletions(-) diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index 1917cd4f449..f0fcb9fb881 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -4740,9 +4740,20 @@ RecoveryRequiresIntParameter(const char *param_name, int currValue, int minValue bool check_primary_slot_name(char **newval, void **extra, GucSource source) { + int err_code; + char *err_msg = NULL; + char *err_hint = NULL; + if (*newval && strcmp(*newval, "") != 0 && - !ReplicationSlotValidateName(*newval, WARNING)) + !ReplicationSlotValidateNameInternal(*newval, &err_code, + &err_msg, &err_hint)) + { + GUC_check_errcode(err_code); + GUC_check_errdetail("%s", err_msg); + if (err_hint != NULL) + GUC_check_errhint("%s", err_hint); return false; + } return true; } diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 94993c677d2..99fe464b99c 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -243,31 +243,62 @@ ReplicationSlotShmemExit(int code, Datum arg) /* * Check whether the passed slot name is valid and report errors at elevel. * + * See comments for ReplicationSlotValidateNameInternal(). + */ +bool +ReplicationSlotValidateName(const char *name, int elevel) +{ + int err_code; + char *err_msg = NULL; + char *err_hint = NULL; + + if (!ReplicationSlotValidateNameInternal(name, + &err_code, &err_msg, &err_hint)) + { + ereport(elevel, + errcode(err_code), + errmsg_internal("%s", err_msg), + (err_hint != NULL) ? errhint("%s", err_hint) : 0); + + pfree(err_msg); + if (err_hint != NULL) + pfree(err_hint); + return false; + } + + return true; +} + +/* + * Check whether the passed slot name is valid. + * * Slot names may consist out of [a-z0-9_]{1,NAMEDATALEN-1} which should allow * the name to be used as a directory name on every supported OS. * - * Returns whether the directory name is valid or not if elevel < ERROR. + * Returns true if the slot name is valid. Otherwise, returns false and stores + * the error code, error message, and optional hint in err_code, err_msg, and + * err_hint, respectively. The caller is responsible for freeing err_msg and + * err_hint, which are palloc'd. */ bool -ReplicationSlotValidateName(const char *name, int elevel) +ReplicationSlotValidateNameInternal(const char *name, + int *err_code, char **err_msg, char **err_hint) { const char *cp; if (strlen(name) == 0) { - ereport(elevel, - (errcode(ERRCODE_INVALID_NAME), - errmsg("replication slot name \"%s\" is too short", - name))); + *err_code = ERRCODE_INVALID_NAME; + *err_msg = psprintf(_("replication slot name \"%s\" is too short"), name); + *err_hint = NULL; return false; } if (strlen(name) >= NAMEDATALEN) { - ereport(elevel, - (errcode(ERRCODE_NAME_TOO_LONG), - errmsg("replication slot name \"%s\" is too long", - name))); + *err_code = ERRCODE_NAME_TOO_LONG; + *err_msg = psprintf(_("replication slot name \"%s\" is too long"), name); + *err_hint = NULL; return false; } @@ -277,11 +308,9 @@ ReplicationSlotValidateName(const char *name, int elevel) || (*cp >= '0' && *cp <= '9') || (*cp == '_'))) { - ereport(elevel, - (errcode(ERRCODE_INVALID_NAME), - errmsg("replication slot name \"%s\" contains invalid character", - name), - errhint("Replication slot names may only contain lower case letters, numbers, and the underscore character."))); + *err_code = ERRCODE_INVALID_NAME; + *err_msg = psprintf(_("replication slot name \"%s\" contains invalid character"), name); + *err_hint = psprintf(_("Replication slot names may only contain lower case letters, numbers, and the underscore character.")); return false; } } diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h index 07561bc4742..43594bb9bef 100644 --- a/src/include/replication/slot.h +++ b/src/include/replication/slot.h @@ -258,6 +258,8 @@ extern void ReplicationSlotMarkDirty(void); /* misc stuff */ extern void ReplicationSlotInitialize(void); extern bool ReplicationSlotValidateName(const char *name, int elevel); +extern bool ReplicationSlotValidateNameInternal(const char *name, + int *err_code, char **err_msg, char **err_hint); extern void ReplicationSlotReserveWal(void); extern void ReplicationSlotsComputeRequiredXmin(bool already_locked); extern void ReplicationSlotsComputeRequiredLSN(void); -- 2.50.1
v6-0001-Make-invalid-primary_slot_name-follow-standard-GU.patch
Description: Binary data
From 15a191136ffd744b970678670454a9d34c73ed80 Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Tue, 21 Oct 2025 14:31:48 +0900 Subject: [PATCH v6] Make invalid primary_slot_name follow standard GUC error reporting. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, if primary_slot_name was set to an invalid slot name and the configuration file was reloaded, both the postmaster and all other backend processes reported a WARNING. With many processes running, this could produce a flood of duplicate messages. The problem was that the GUC check hook for primary_slot_name reported errors at WARNING level via ereport(). This commit changes the check hook to use GUC_check_errdetail() and GUC_check_errhint() for error reporting. As with other GUC parameters, this causes non-postmaster processes to log the message at DEBUG3, so by default, only the postmaster's message appears in the log file. Backpatch to all supported versions. Author: Fujii Masao <[email protected]> Reviewed-by: Chao Li <[email protected]> Reviewed-by: Amit Kapila <[email protected]> Reviewed-by: Álvaro Herrera <[email protected]> Discussion: https://postgr.es/m/cahgqgwfud-cvthctfusbfkhbs6jj6kdaptdlwkvp2qjux6l...@mail.gmail.com Backpatch-through: 13 --- src/backend/replication/slot.c | 59 +++++++++++++++++++++++++--------- src/backend/utils/misc/guc.c | 13 +++++++- src/include/replication/slot.h | 2 ++ 3 files changed, 58 insertions(+), 16 deletions(-) diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index c59f249fb61..0d16c791116 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -188,31 +188,62 @@ ReplicationSlotShmemExit(int code, Datum arg) /* * Check whether the passed slot name is valid and report errors at elevel. * + * See comments for ReplicationSlotValidateNameInternal(). + */ +bool +ReplicationSlotValidateName(const char *name, int elevel) +{ + int err_code; + char *err_msg = NULL; + char *err_hint = NULL; + + if (!ReplicationSlotValidateNameInternal(name, + &err_code, &err_msg, &err_hint)) + { + ereport(elevel, + errcode(err_code), + errmsg_internal("%s", err_msg), + (err_hint != NULL) ? errhint("%s", err_hint) : 0); + + pfree(err_msg); + if (err_hint != NULL) + pfree(err_hint); + return false; + } + + return true; +} + +/* + * Check whether the passed slot name is valid. + * * Slot names may consist out of [a-z0-9_]{1,NAMEDATALEN-1} which should allow * the name to be used as a directory name on every supported OS. * - * Returns whether the directory name is valid or not if elevel < ERROR. + * Returns true if the slot name is valid. Otherwise, returns false and stores + * the error code, error message, and optional hint in err_code, err_msg, and + * err_hint, respectively. The caller is responsible for freeing err_msg and + * err_hint, which are palloc'd. */ bool -ReplicationSlotValidateName(const char *name, int elevel) +ReplicationSlotValidateNameInternal(const char *name, + int *err_code, char **err_msg, char **err_hint) { const char *cp; if (strlen(name) == 0) { - ereport(elevel, - (errcode(ERRCODE_INVALID_NAME), - errmsg("replication slot name \"%s\" is too short", - name))); + *err_code = ERRCODE_INVALID_NAME; + *err_msg = psprintf(_("replication slot name \"%s\" is too short"), name); + *err_hint = NULL; return false; } if (strlen(name) >= NAMEDATALEN) { - ereport(elevel, - (errcode(ERRCODE_NAME_TOO_LONG), - errmsg("replication slot name \"%s\" is too long", - name))); + *err_code = ERRCODE_NAME_TOO_LONG; + *err_msg = psprintf(_("replication slot name \"%s\" is too long"), name); + *err_hint = NULL; return false; } @@ -222,11 +253,9 @@ ReplicationSlotValidateName(const char *name, int elevel) || (*cp >= '0' && *cp <= '9') || (*cp == '_'))) { - ereport(elevel, - (errcode(ERRCODE_INVALID_NAME), - errmsg("replication slot name \"%s\" contains invalid character", - name), - errhint("Replication slot names may only contain lower case letters, numbers, and the underscore character."))); + *err_code = ERRCODE_INVALID_NAME; + *err_msg = psprintf(_("replication slot name \"%s\" contains invalid character"), name); + *err_hint = psprintf(_("Replication slot names may only contain lower case letters, numbers, and the underscore character.")); return false; } } diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index abfc581171a..a7f5ad95619 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -13044,9 +13044,20 @@ assign_recovery_target_lsn(const char *newval, void *extra) static bool check_primary_slot_name(char **newval, void **extra, GucSource source) { + int err_code; + char *err_msg = NULL; + char *err_hint = NULL; + if (*newval && strcmp(*newval, "") != 0 && - !ReplicationSlotValidateName(*newval, WARNING)) + !ReplicationSlotValidateNameInternal(*newval, &err_code, + &err_msg, &err_hint)) + { + GUC_check_errcode(err_code); + GUC_check_errdetail("%s", err_msg); + if (err_hint != NULL) + GUC_check_errhint("%s", err_hint); return false; + } return true; } diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h index deba2c4e499..51dfb740cfa 100644 --- a/src/include/replication/slot.h +++ b/src/include/replication/slot.h @@ -208,6 +208,8 @@ extern void ReplicationSlotMarkDirty(void); /* misc stuff */ extern void ReplicationSlotInitialize(void); extern bool ReplicationSlotValidateName(const char *name, int elevel); +extern bool ReplicationSlotValidateNameInternal(const char *name, + int *err_code, char **err_msg, char **err_hint); extern void ReplicationSlotReserveWal(void); extern void ReplicationSlotsComputeRequiredXmin(bool already_locked); extern void ReplicationSlotsComputeRequiredLSN(void); -- 2.50.1
From 31e9f754f04c0fc76f87ba44bad045a56ab10f93 Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Tue, 21 Oct 2025 14:08:56 +0900 Subject: [PATCH v6] Make invalid primary_slot_name follow standard GUC error reporting. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, if primary_slot_name was set to an invalid slot name and the configuration file was reloaded, both the postmaster and all other backend processes reported a WARNING. With many processes running, this could produce a flood of duplicate messages. The problem was that the GUC check hook for primary_slot_name reported errors at WARNING level via ereport(). This commit changes the check hook to use GUC_check_errdetail() and GUC_check_errhint() for error reporting. As with other GUC parameters, this causes non-postmaster processes to log the message at DEBUG3, so by default, only the postmaster's message appears in the log file. Backpatch to all supported versions. Author: Fujii Masao <[email protected]> Reviewed-by: Chao Li <[email protected]> Reviewed-by: Amit Kapila <[email protected]> Reviewed-by: Álvaro Herrera <[email protected]> Discussion: https://postgr.es/m/cahgqgwfud-cvthctfusbfkhbs6jj6kdaptdlwkvp2qjux6l...@mail.gmail.com Backpatch-through: 13 --- src/backend/access/transam/xlogrecovery.c | 13 ++++- src/backend/replication/slot.c | 65 +++++++++++++++++------ src/include/replication/slot.h | 2 + 3 files changed, 64 insertions(+), 16 deletions(-) diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index efbe77a5747..d5d7afb7206 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -4760,9 +4760,20 @@ RecoveryRequiresIntParameter(const char *param_name, int currValue, int minValue bool check_primary_slot_name(char **newval, void **extra, GucSource source) { + int err_code; + char *err_msg = NULL; + char *err_hint = NULL; + if (*newval && strcmp(*newval, "") != 0 && - !ReplicationSlotValidateName(*newval, WARNING)) + !ReplicationSlotValidateNameInternal(*newval, &err_code, + &err_msg, &err_hint)) + { + GUC_check_errcode(err_code); + GUC_check_errdetail("%s", err_msg); + if (err_hint != NULL) + GUC_check_errhint("%s", err_hint); return false; + } return true; } diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 23c616b1d7d..bb3a5ad074a 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -258,31 +258,68 @@ ReplicationSlotShmemExit(int code, Datum arg) /* * Check whether the passed slot name is valid and report errors at elevel. * + * See comments for ReplicationSlotValidateNameInternal(). + */ +bool +ReplicationSlotValidateName(const char *name, int elevel) +{ + int err_code; + char *err_msg = NULL; + char *err_hint = NULL; + + if (!ReplicationSlotValidateNameInternal(name, + &err_code, &err_msg, &err_hint)) + { + /* + * Use errmsg_internal() and errhint_internal() instead of errmsg() + * and errhint(), since the messages from + * ReplicationSlotValidateNameInternal() are already translated. This + * avoids double translation. + */ + ereport(elevel, + errcode(err_code), + errmsg_internal("%s", err_msg), + (err_hint != NULL) ? errhint_internal("%s", err_hint) : 0); + + pfree(err_msg); + if (err_hint != NULL) + pfree(err_hint); + return false; + } + + return true; +} + +/* + * Check whether the passed slot name is valid. + * * Slot names may consist out of [a-z0-9_]{1,NAMEDATALEN-1} which should allow * the name to be used as a directory name on every supported OS. * - * Returns whether the directory name is valid or not if elevel < ERROR. + * Returns true if the slot name is valid. Otherwise, returns false and stores + * the error code, error message, and optional hint in err_code, err_msg, and + * err_hint, respectively. The caller is responsible for freeing err_msg and + * err_hint, which are palloc'd. */ bool -ReplicationSlotValidateName(const char *name, int elevel) +ReplicationSlotValidateNameInternal(const char *name, + int *err_code, char **err_msg, char **err_hint) { const char *cp; if (strlen(name) == 0) { - ereport(elevel, - (errcode(ERRCODE_INVALID_NAME), - errmsg("replication slot name \"%s\" is too short", - name))); + *err_code = ERRCODE_INVALID_NAME; + *err_msg = psprintf(_("replication slot name \"%s\" is too short"), name); + *err_hint = NULL; return false; } if (strlen(name) >= NAMEDATALEN) { - ereport(elevel, - (errcode(ERRCODE_NAME_TOO_LONG), - errmsg("replication slot name \"%s\" is too long", - name))); + *err_code = ERRCODE_NAME_TOO_LONG; + *err_msg = psprintf(_("replication slot name \"%s\" is too long"), name); + *err_hint = NULL; return false; } @@ -292,11 +329,9 @@ ReplicationSlotValidateName(const char *name, int elevel) || (*cp >= '0' && *cp <= '9') || (*cp == '_'))) { - ereport(elevel, - (errcode(ERRCODE_INVALID_NAME), - errmsg("replication slot name \"%s\" contains invalid character", - name), - errhint("Replication slot names may only contain lower case letters, numbers, and the underscore character."))); + *err_code = ERRCODE_INVALID_NAME; + *err_msg = psprintf(_("replication slot name \"%s\" contains invalid character"), name); + *err_hint = psprintf(_("Replication slot names may only contain lower case letters, numbers, and the underscore character.")); return false; } } diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h index 76aeeb92242..d9aab934048 100644 --- a/src/include/replication/slot.h +++ b/src/include/replication/slot.h @@ -293,6 +293,8 @@ extern void ReplicationSlotMarkDirty(void); /* misc stuff */ extern void ReplicationSlotInitialize(void); extern bool ReplicationSlotValidateName(const char *name, int elevel); +extern bool ReplicationSlotValidateNameInternal(const char *name, + int *err_code, char **err_msg, char **err_hint); extern void ReplicationSlotReserveWal(void); extern void ReplicationSlotsComputeRequiredXmin(bool already_locked); extern void ReplicationSlotsComputeRequiredLSN(void); -- 2.50.1
