Horton

Thread

Commits

  1. Make invalid primary_slot_name follow standard GUC error reporting.

  1. Invalid primary_slot_name triggers warnings in all processes on reload

    Fujii Masao <masao.fujii@gmail.com> — 2025-09-12T12:12:09Z

    Hi,

While reviewing the patch at [1], I noticed that if primary_slot_name is
set to an invalid slot name in postgresql.conf and the configuration file
is reloaded, all running postgres processes emit the WARNING message
as follows. Isn't this a bug?

To fix this issue, GUC_check_errmsg() should be used instead of ereport()
when an invalid slot name is found in the setting. Thoughts?

------------------------------------
$ initdb -D data
$ pg_ctl -D data start
$ psql <<EOF
ALTER SYSTEM SET log_line_prefix TO '[%b] ';
SELECT pg_reload_conf();
\! echo "primary_slot_name = 'invalid-'" >> data/postgresql.conf
SELECT pg_reload_conf();
EOF


[postmaster] WARNING:  replication slot name "invalid-" contains
invalid character
[postmaster] HINT:  Replication slot names may only contain lower case
letters, numbers, and the underscore character.
[postmaster] LOG:  invalid value for parameter "primary_slot_name": "invalid-"
[postmaster] LOG:  configuration file
"/System/Volumes/Data/dav/head-pgsql/data/postgresql.conf" contains
errors; unaffected changes were applied
[logical replication launcher] WARNING:  replication slot name
"invalid-" contains invalid character
[logical replication launcher] HINT:  Replication slot names may only
contain lower case letters, numbers, and the underscore character.
[checkpointer] WARNING:  replication slot name "invalid-" contains
invalid character
[checkpointer] HINT:  Replication slot names may only contain lower
case letters, numbers, and the underscore character.
[walwriter] WARNING:  replication slot name "invalid-" contains
invalid character
[walwriter] HINT:  Replication slot names may only contain lower case
letters, numbers, and the underscore character.
hrk:head-pgsql postgres$ [background writer] WARNING:  replication
slot name "invalid-" contains invalid character
[background writer] HINT:  Replication slot names may only contain
lower case letters, numbers, and the underscore character.
[io worker] WARNING:  replication slot name "invalid-" contains
invalid character
[io worker] HINT:  Replication slot names may only contain lower case
letters, numbers, and the underscore character.
[io worker] WARNING:  replication slot name "invalid-" contains
invalid character
[io worker] HINT:  Replication slot names may only contain lower case
letters, numbers, and the underscore character.
[autovacuum launcher] WARNING:  replication slot name "invalid-"
contains invalid character
[autovacuum launcher] HINT:  Replication slot names may only contain
lower case letters, numbers, and the underscore character.
[io worker] WARNING:  replication slot name "invalid-" contains
invalid character
[io worker] HINT:  Replication slot names may only contain lower case
letters, numbers, and the underscore character.
------------------------------------

Regards,

[1] http://postgr.es/m/CANhcyEUUWJ7XBycqFuqQg=eZ_==KT6S7E+rdMy-GE23oLa8tmQ@mail.gmail.com


-- 
Fujii Masao

  2. Re: Invalid primary_slot_name triggers warnings in all processes on reload

    Fujii Masao <masao.fujii@gmail.com> — 2025-09-18T13:54:46Z

    On Fri, Sep 12, 2025 at 9:12 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> Hi,
>
> While reviewing the patch at [1], I noticed that if primary_slot_name is
> set to an invalid slot name in postgresql.conf and the configuration file
> is reloaded, all running postgres processes emit the WARNING message
> as follows. Isn't this a bug?
>
> To fix this issue, GUC_check_errmsg() should be used instead of ereport()
> when an invalid slot name is found in the setting. Thoughts?

Another simple fix would be to have processes other than the postmaster
report invalid primary_slot_name at DEBUG3 instead of WARNING.
In that case, only the postmaster reports it at WARNING, so by default
only that message appears in the log file. This matches the behavior for
other GUC parameters with invalid settings.

I've attached a patch implementing this approach. Thought?

-- 
Fujii Masao

  3. Re: Invalid primary_slot_name triggers warnings in all processes on reload

    Fujii Masao <masao.fujii@gmail.com> — 2025-09-18T16:48:27Z

    On Thu, Sep 18, 2025 at 10:54 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Fri, Sep 12, 2025 at 9:12 PM Fujii Masao <masao.fujii@gmail.com> wrote:
> >
> > Hi,
> >
> > While reviewing the patch at [1], I noticed that if primary_slot_name is
> > set to an invalid slot name in postgresql.conf and the configuration file
> > is reloaded, all running postgres processes emit the WARNING message
> > as follows. Isn't this a bug?
> >
> > To fix this issue, GUC_check_errmsg() should be used instead of ereport()
> > when an invalid slot name is found in the setting. Thoughts?
>
> Another simple fix would be to have processes other than the postmaster
> report invalid primary_slot_name at DEBUG3 instead of WARNING.
> In that case, only the postmaster reports it at WARNING, so by default
> only that message appears in the log file. This matches the behavior for
> other GUC parameters with invalid settings.
>
> I've attached a patch implementing this approach. Thought?

This approach unexpectedly suppresses the log message about an invalid
primary_slot_name when it's set with ALTER SYSTEM SET, so it isn't suitable.
Instead, we should use GUC_check_xxx() functions for error reporting
in the GUC check hook.

I've attached a patch that updates the check hook for primary_slot_name
to use GUC_check_errdetail() and GUC_check_errhint(). As with other
GUC parameters, this makes non-postmaster processes log at DEBUG3,
so by default only the postmaster's message appears in the log file.

Regards,

-- 
Fujii Masao

  4. Re: Invalid primary_slot_name triggers warnings in all processes on reload

    Chao Li <li.evan.chao@gmail.com> — 2025-09-18T23:21:06Z

    
> On Sep 19, 2025, at 00:48, Fujii Masao <masao.fujii@gmail.com> wrote:
> 
> Fujii Masao
> <v2-0001-Make-invalid-primary_slot_name-follow-standard-GU.patch>


```
+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));
+
+	return false;
```

Do we need to free memory pointed by err_msg and err_hint that were allocated from psprintf()? The code comment of psprintf() says caller is responsible for free the memory.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  5. Re: Invalid primary_slot_name triggers warnings in all processes on reload

    Fujii Masao <masao.fujii@gmail.com> — 2025-09-19T06:30:11Z

    On Fri, Sep 19, 2025 at 8:21 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> On Sep 19, 2025, at 00:48, Fujii Masao <masao.fujii@gmail.com> wrote:
>
> Fujii Masao
> <v2-0001-Make-invalid-primary_slot_name-follow-standard-GU.patch>
>
>
> ```
> +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));
> +
> + return false;
> ```
>
> Do we need to free memory pointed by err_msg and err_hint that were allocated from psprintf()? The code comment of psprintf() says caller is responsible for free the memory.

Thanks for the review!

I had thought that since the short-lived "config file processing" memory context
is used when processing the configuration file and then freed afterward,
there was no need for ReplicationSlotValidateName() to call pfree().
However, when it's called by StartupReorderBuffer() with elevel = DEBUG2,
allocations seem to be made in TopMemoryContext, so they could persist
much longer.

So I agree it's safer to free them explicitly. In the attached updated patch,
ReplicationSlotValidateName() now pfrees err_msg and err_hint when needed.

Regards,

-- 
Fujii Masao

  6. Re: Invalid primary_slot_name triggers warnings in all processes on reload

    Amit Kapila <amit.kapila16@gmail.com> — 2025-09-23T04:23:55Z

    On Fri, Sep 19, 2025 at 12:00 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> So I agree it's safer to free them explicitly. In the attached updated patch,
> ReplicationSlotValidateName() now pfrees err_msg and err_hint when needed.
>

+error:
+ if (elevel == 0)
+ {
+ GUC_check_errdetail("%s", err_msg);
+ if (err_hint != NULL)
+ GUC_check_errhint("%s", err_hint);

I see that other places use GUC_check_errcode. See
check_synchronous_standby_names. So, shouldn't we use it here as well?

I don't see any other place distinguishing GUC related errors in this
way. It seems the other way to differentiate throwing errors for GUC
related messages is used in call_string_check_hook and friends. It may
not be used as it is but it can give some ideas to explore. I have not
explored in detail so it may not be relevant here but it is worth
checking once.

-- 
With Regards,
Amit Kapila.

  7. Re: Invalid primary_slot_name triggers warnings in all processes on reload

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-09-23T15:11:04Z

    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)

  8. Re: Invalid primary_slot_name triggers warnings in all processes on reload

    Fujii Masao <masao.fujii@gmail.com> — 2025-09-24T18:26:45Z

    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

  9. Re: Invalid primary_slot_name triggers warnings in all processes on reload

    Amit Kapila <amit.kapila16@gmail.com> — 2025-10-09T05:06:36Z

    On Wed, Sep 24, 2025 at 11:57 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> 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.
>

Are you still planning to work on it? I am asking because it is better
to fix this before merging the change for another somewhat related
bug-fix patch being discussed in thread [1].

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

Right. I think we can consider this separately just as a HEAD patch.

[1] - https://www.postgresql.org/message-id/CAA5-nLB0JhVO-ykpfguxyGkoAk1tECi5xMTqAoVJ6spnDiQzaw%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

  10. Re: Invalid primary_slot_name triggers warnings in all processes on reload

    Fujii Masao <masao.fujii@gmail.com> — 2025-10-09T05:26:21Z

    On Thu, Oct 9, 2025 at 2:06 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> Are you still planning to work on it?

Yes, thanks for the ping!
Attached is the updated version of the patch.

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

This isn't directly related to this thread, but while updating the patch,
I noticed that the call_xxx_check_hook() functions in guc.c use errhint()
instead of errhint_internal(). That might also cause double translation.
Interestingly, for log and detail messages they already use errmsg_internal()
and errdetail_internal(). Only the HINT messages still use the non-internal
version. Should we switch to use errhint_internal() as well?


> I am asking because it is better
> to fix this before merging the change for another somewhat related
> bug-fix patch being discussed in thread [1].

+1

> > 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.
> >
>
> Right. I think we can consider this separately just as a HEAD patch.

+1

Regards,

-- 
Fujii Masao

  11. Re: Invalid primary_slot_name triggers warnings in all processes on reload

    Fujii Masao <masao.fujii@gmail.com> — 2025-10-10T05:52:02Z

    On Thu, Oct 9, 2025 at 2:26 PM Fujii Masao <masao.fujii@gmail.com> 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.

0002 adds GUC_check_errmsg_internal(), GUC_check_errdetail_internal(),
and GUC_check_errhint_internal(). These work like their counterparts
(e.g., GUC_check_errmsg()) but skip translation. It also updates
the check hook for primary_slot_name to use these new functions
when handling already-translated error messages.

Regards,

-- 
Fujii Masao

  12. Re: Invalid primary_slot_name triggers warnings in all processes on reload

    Fujii Masao <masao.fujii@gmail.com> — 2025-10-21T06:11:26Z

    On Fri, Oct 10, 2025 at 2:52 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Thu, Oct 9, 2025 at 2:26 PM Fujii Masao <masao.fujii@gmail.com> 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

  13. RE: Invalid primary_slot_name triggers warnings in all processes on reload

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-10-21T08:00:47Z

    Dear Fujii-san,

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

FYI, the patch could not be applied cleanly for PG13 and 14 for my env:

```
postgres (REL_13_STABLE $%=)$ git am ../patches/primary_slot_name/v6-0001-PG13-PG15-Make-invalid-primary_slot_name-follow-standard-GU.txt
Applying: Make invalid primary_slot_name follow standard GUC error reporting.
error: patch failed: src/include/replication/slot.h:208
error: src/include/replication/slot.h: patch does not apply
...
```

Cosmetic comments:

```
+       if (!ReplicationSlotValidateNameInternal(name,
+                                                                                        &err_code, &err_msg, &err_hint))
...
-ReplicationSlotValidateName(const char *name, int elevel)
+ReplicationSlotValidateNameInternal(const char *name,
+                                                                       int *err_code, char **err_msg, char **err_hint)
```

Patches for older branches have strange indent, maybe because
"bool allow_reserved_name" is just removed. Should we move up arguments?

> 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?

Personally considered it can be added...

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  14. Re: Invalid primary_slot_name triggers warnings in all processes on reload

    Fujii Masao <masao.fujii@gmail.com> — 2025-10-21T08:47:30Z

    On Tue, Oct 21, 2025 at 5:00 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Fujii-san,

Thanks for testing and reviewing!


> > 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.
>
> FYI, the patch could not be applied cleanly for PG13 and 14 for my env:

I first applied the patch to v15, then used git cherry-pick to backpatch it
to v14 and v13 without any issues. You can probably do the same to apply it
to those branches.


> Cosmetic comments:
>
> ```
> +       if (!ReplicationSlotValidateNameInternal(name,
> +                                                                                        &err_code, &err_msg, &err_hint))
> ...
> -ReplicationSlotValidateName(const char *name, int elevel)
> +ReplicationSlotValidateNameInternal(const char *name,
> +                                                                       int *err_code, char **err_msg, char **err_hint)
> ```
>
> Patches for older branches have strange indent, maybe because
> "bool allow_reserved_name" is just removed. Should we move up arguments?

Since pgindent doesn't treat the current indentation as an issue,
I'm fine keeping it as is, though I don't mind changing it if you think
it's worth updating.


> > 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?
>
> Personally considered it can be added...

Just to confirm - you'd prefer backpatching errhint_internal() to v17 and
earlier branches, and then updating the patch to use it to avoid double
translation, right?

Regards,

-- 
Fujii Masao

  15. RE: Invalid primary_slot_name triggers warnings in all processes on reload

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-10-21T09:06:52Z

    Dear Fujii-san,

> I first applied the patch to v15, then used git cherry-pick to backpatch it
> to v14 and v13 without any issues. You can probably do the same to apply it
> to those branches.

I did same approach and confirmed it could be applied well.

> > Cosmetic comments:
> >
> > ```
> > +       if (!ReplicationSlotValidateNameInternal(name,
> > +
> &err_code, &err_msg, &err_hint))
> > ...
> > -ReplicationSlotValidateName(const char *name, int elevel)
> > +ReplicationSlotValidateNameInternal(const char *name,
> > +
> int *err_code, char **err_msg, char **err_hint)
> > ```
> >
> > Patches for older branches have strange indent, maybe because
> > "bool allow_reserved_name" is just removed. Should we move up arguments?
> 
> Since pgindent doesn't treat the current indentation as an issue,
> I'm fine keeping it as is, though I don't mind changing it if you think
> it's worth updating.

I do not have strong opinion neither, but I still think it can be updated.

> Just to confirm - you'd prefer backpatching errhint_internal() to v17 and
> earlier branches, and then updating the patch to use it to avoid double
> translation, right?

Exactly, but I want to ask other Seniors as well.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  16. Re: Invalid primary_slot_name triggers warnings in all processes on reload

    Amit Kapila <amit.kapila16@gmail.com> — 2025-10-21T10:27:46Z

    On Tue, Oct 21, 2025 at 2:36 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> > Just to confirm - you'd prefer backpatching errhint_internal() to v17 and
> > earlier branches, and then updating the patch to use it to avoid double
> > translation, right?
>
> Exactly, but I want to ask other Seniors as well.
>

I don't think it is important enough to backpatch errhint_internal to
v17 and earlier branches. We can let double translation happen with
the use of errhint or we can even consider this as a HEAD only
improvement because this is not a blocking issue even when it happens.

-- 
With Regards,
Amit Kapila.

  17. Re: Invalid primary_slot_name triggers warnings in all processes on reload

    Fujii Masao <masao.fujii@gmail.com> — 2025-10-22T03:52:07Z

    On Tue, Oct 21, 2025 at 7:27 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Oct 21, 2025 at 2:36 PM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> > > Just to confirm - you'd prefer backpatching errhint_internal() to v17 and
> > > earlier branches, and then updating the patch to use it to avoid double
> > > translation, right?
> >
> > Exactly, but I want to ask other Seniors as well.
> >
>
> I don't think it is important enough to backpatch errhint_internal to
> v17 and earlier branches. We can let double translation happen with
> the use of errhint or we can even consider this as a HEAD only
> improvement because this is not a blocking issue even when it happens.

+1

So let's push the current patch, including only the cosmetic indentation
changes suggested by Hayato-san. *If* we later reach consensus on
backpatching errhint_internal(), we can handle that separately.

Since another patch [1] seems to depend on this one, it's better not to
delay committing it.

Regards,

[1] https://postgr.es/m/CAA5-nLCeO4MQzWipCXH58qf0arruiw0OeUc1+Q=Z=4GM+=v1NQ@mail.gmail.com

-- 
Fujii Masao

  18. Re: Invalid primary_slot_name triggers warnings in all processes on reload

    Fujii Masao <masao.fujii@gmail.com> — 2025-10-22T13:31:22Z

    On Wed, Oct 22, 2025 at 12:52 PM Fujii Masao <masao.fujii@gmail.com> wrote:
> So let's push the current patch, including only the cosmetic indentation
> changes suggested by Hayato-san.

I've pushed the patch. Thanks!

Attached is the rebased version of the patch I shared earlier, which adds
GUC_check_errmsg_internal(), GUC_check_errdetail_internal(),
and GUC_check_errhint_internal(). These functions work like
their counterparts (e.g., GUC_check_errmsg()) but skip translation.
The patch also updates the check hook for primary_slot_name to use
these new functions when handling already-translated error messages.
If we apply this patch, I'm thinking to push it to master only.

Regards,

-- 
Fujii Masao