Обсуждение: [PATCH] Add hints for invalid binary encoding names in encode/decode functions

On Sat, Nov 8, 2025 at 3:26 PM Sugamoto Shinya <shinya34892@gmail.com> wrote:
>
> Hi,
>
> When users pass an invalid encoding name to the built-in functions
> `encode()` or `decode()`, PostgreSQL currently reports only:
>
>     ERROR:  unrecognized encoding: "xxx"
>
> This patch adds an error hint listing the valid encoding names,
> so users can immediately see the supported options.
>
> Example:
>
>     SELECT encode('\x01'::bytea, 'invalid');
>     ERROR:  unrecognized encoding: "invalid"
>     HINT:  Valid binary encodings are: "hex", "base64", "base64url", "escape".
>
> This change applies to both `binary_encode()` and `binary_decode()` in `encode.c`,
> and adds regression tests under `src/test/regress/sql/strings.sql`.

+1


- errmsg("unrecognized encoding: \"%s\"", namebuf)));
+ errmsg("unrecognized encoding: \"%s\"", namebuf),
+ errhint("Valid binary encodings are: %s",
+ "\"hex\", \"base64\", \"base64url\", \"escape\".")));

Since neither encode.c nor the documentation for encode() use
the term "binary encoding", I think just "encodings" is sufficient
in the hint message.

Also, the colon after "encodings”"doesn't seem necessary.


+-- invalid encoding names should report the valid options
+SELECT encode('\x01'::bytea, 'invalid');  -- error

I'd like to improve the comment like:
"report an error with a hint listing valid encodings when an invalid
encoding is specified".


> Although this is a small change, let me share a few considerations behind it:
>
> - I extracted the valid encodings from the hint messages and used a format specifier like
>   `Valid binary encodings are: %s`, so that we avoid scattering those fixed strings
>   across translation files.

I understand your point. But since new encodings are rarely added as you told,
I'm fine to list them directly in the hint instead of using %s,
similar to other hints like:

src/backend/commands/dbcommands.c:
errhint("Valid strategies are \"wal_log\" and \"file_copy\".")));

Regards,

--
Fujii Masao

> On Nov 8, 2025, at 14:25, Sugamoto Shinya <shinya34892@gmail.com> wrote:
>
> Hi,
>
> When users pass an invalid encoding name to the built-in functions
> `encode()` or `decode()`, PostgreSQL currently reports only:
>
>     ERROR:  unrecognized encoding: "xxx"
>
> This patch adds an error hint listing the valid encoding names,
> so users can immediately see the supported options.
>
> Example:
>
>     SELECT encode('\x01'::bytea, 'invalid');
>     ERROR:  unrecognized encoding: "invalid"
>     HINT:  Valid binary encodings are: "hex", "base64", "base64url", "escape".
>
> This change applies to both `binary_encode()` and `binary_decode()` in `encode.c`,
> and adds regression tests under `src/test/regress/sql/strings.sql`.
>
> Although this is a small change, let me share a few considerations behind it:
>
> - I extracted the valid encodings from the hint messages and used a format specifier like
>   `Valid binary encodings are: %s`, so that we avoid scattering those fixed strings
>   across translation files.
> - I briefly considered adding a small helper function to build the hint string,
>   but decided against it since keeping the codebase simple felt preferable, and
>   new binary encodings are not added very often.
>
> I’d be happy to hear any opinions or suggestions.
>
> Patch attached.
>
> Best regards,
> Shinya Sugamoto
> <0001-Added-error-hints-for-invalid-binary-encoding-names-.patch>

1.
```
-                 errmsg("unrecognized encoding: \"%s\"", namebuf)));
+                 errmsg("unrecognized encoding: \"%s\"", namebuf),
+                 errhint("Valid binary encodings are: %s",
+                         "\"hex\", \"base64\", \"base64url\", \"escape\".")));
```

I think hardcoding the encoding list is fragile. AFAIK, “base64url” was newly added just a couple of months ago.

The list is defined in encode.c (search for enclist in the file), I guess we can add a function to return a string with
aencoding names. 

2
```
+                 errhint("Valid binary encodings are: %s",
+                         "\"hex\", \"base64\", \"base64url\", \"escape\".")));
```

Looks like putting punctuation inside %s is not normal. By looking at other hint messages, they usually do something
like:

```
errhint("Perhaps you meant the option \"%s\".”, value)
```

But I think if you address comment 1, then this one will be addressed as well.

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

> On 10 Nov 2025, at 10:06, Chao Li <li.evan.chao@gmail.com> wrote:
>> On Nov 8, 2025, at 14:25, Sugamoto Shinya <shinya34892@gmail.com> wrote:

>> This patch adds an error hint listing the valid encoding names,
>> so users can immediately see the supported options.

+1.

> I think hardcoding the encoding list is fragile. AFAIK, “base64url” was newly added just a couple of months ago.
>
> The list is defined in encode.c (search for enclist in the file), I guess we can add a function to return a string
witha encoding names. 

New encodings are added very infrequently, we can revisit this if that changes
but till then I think the simplicity of a hardcoded string is preferrable.

--
Daniel Gustafsson

> On Nov 10, 2025, at 22:43, Daniel Gustafsson <daniel@yesql.se> wrote:
>
>> On 10 Nov 2025, at 10:06, Chao Li <li.evan.chao@gmail.com> wrote:
>>> On Nov 8, 2025, at 14:25, Sugamoto Shinya <shinya34892@gmail.com> wrote:
>
>>> This patch adds an error hint listing the valid encoding names,
>>> so users can immediately see the supported options.
>
> +1.
>
>> I think hardcoding the encoding list is fragile. AFAIK, “base64url” was newly added just a couple of months ago.
>>
>> The list is defined in encode.c (search for enclist in the file), I guess we can add a function to return a string
witha encoding names. 
>
> New encodings are added very infrequently, we can revisit this if that changes
> but till then I think the simplicity of a hardcoded string is preferrable.
>

I’m not convinced that a hardcoded string is actually better. In fact, the less often something changes, the easier it
isto miss updating it when we need to. 

If we add a helper function to build the list of supported encodings:

* The function is small and straightforward — trivial to implement.
* The function doesn’t run in any performance-critical paths, so no meaningful cost there.
* The encoding names are plain ASCII identifiers and effectively the same across languages, so there’s no real benefit
tohardcoding them for i18n purposes either. 

So I still think the helper function seems cleaner and less error-prone than a hardcoded string.

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

On Tue, Nov 11, 2025 at 7:14 AM Sugamoto Shinya <shinya34892@gmail.com> wrote:
>
> Thanks everyone for checking my proposal.
> Other than how to make the hint string, every comment looks good to me. I'll take those into my patch. I appreciate
it.
>
>
> Given the comments you all made, I looked into the source code to see how we build `errhint` with values, and found
thefollowing. 
>
>  // contrib/pg_prewarm/pg_prewarm.c:102
>  errhint("Valid prewarm types are \"prefetch\", \"read\", and \"buffer\".")
>
>  // contrib/amcheck/verify_heapam.c:303
>  errhint("Valid skip options are \"all-visible\", \"all-frozen\", and \"none\".")
>
>  // src/backend/replication/logical/launcher.c:458
>   errhint("You might need to increase \"%s\".", "max_logical_replication_workers")
>
> // src/backend/commands/opclasscmds.c:1240
>   errhint("Valid signature of operator class options parsing function is %s.",
>           "(internal) RETURNS void")));
>
> // src/backend/commands/dbcommands.c:1044
>   errhint("Valid strategies are \"wal_log\" and \"file_copy\".")));
>
>
> Regarding the following comment,
>
> >In fact, the less often something changes, the easier it is to miss updating it when we need to.
> >
> > If we add a helper function to build the list of supported encodings:
> >
> > * The function is small and straightforward — trivial to implement.
> > * The function doesn’t run in any performance-critical paths, so no meaningful cost there.
> > * The encoding names are plain ASCII identifiers and effectively the same across languages, so there’s no real
benefitto hardcoding them for i18n purposes either. 
>
> While I partly agree with the idea of adding a helper function, introducing one just for constructing a simple hint
stringwould lead to more code of the kind that I would prefer to avoid, since this is not our main concern. 
>
> So, although it is true that having such a helper function would improve maintainability in some sense,
> considering that:
> - there are already several existing cases where values are simply embedded into hint texts (as Fujii pointed out),
and
> - this approach does not violate our coding rules or make the code harder to understand,
> I tend to think that it’s acceptable to hard-code the list of possible encodings in the hint this time.
>
> Thoughts?

+1 for hard-coding the supported encoding list. I think it's a good
start point. We can revisit the idea of dynamically constructing the
encoding list if we're concerned about its maintenance cost.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Wed, Nov 12, 2025 at 10:10 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> +1 for hard-coding the supported encoding list. I think it's a good
> start point. We can revisit the idea of dynamically constructing the
> encoding list if we're concerned about its maintenance cost.

+1

Regards,

--
Fujii Masao

On 10.11.25 10:06, Chao Li wrote:
> The list is defined in encode.c (search for enclist in the file), I guess we can add a function to return a string
witha encoding names.
 
> 
> 2
> ```
> +                 errhint("Valid binary encodings are: %s",
> +                         "\"hex\", \"base64\", \"base64url\", \"escape\".")));
> ```
> 
> Looks like putting punctuation inside %s is not normal. By looking at other hint messages, they usually do something
like:
> 
> ```
> errhint("Perhaps you meant the option \"%s\".”, value)
> ```

Yes, this is because the punctuation is itself subject to translation. 
(Most obviously, different languages use different quotation marks.)  So 
the preferred style is something like

errhint("Valid encodings are: \"%s\", \"%s\", and \"%s\", "hex", ...)

or however many you need.

See also 
<https://www.postgresql.org/message-id/202511070936.jj4o4ktd4b6l%40alvherre.pgsql> 
for a related discussion.

On Wed, Nov 12, 2025 at 10:23 PM Sugamoto Shinya <shinya34892@gmail.com> wrote:
> Thanks everyone for reviewing my proposal.
> I've hard-coded the valid list of encoding names.
>
> Also, I modified the hint string into like "Valid encodings are \"hex\", \"base64\", \"base64url\", and \"escape\"."
> by adding "and" before "escape".
>
> I attached my v2 patch to this message. Please let me know freely if you have any additional questions.

Thanks for the updated patch! LGTM.

One minor comment: in v2 patch, it seems the encodings in the HINT message are
listed in the order they appear in the enclist struct. Wouldn't it be
clearer to list them
alphabetically, matching the order shown in the docs [1]: base64,
base64url, escape, and hex?

Regards,

[1] https://www.postgresql.org/docs/devel/functions-binarystring.html#ENCODE-FORMAT-BASE64

--
Fujii Masao

> On 17 Nov 2025, at 15:08, Sugamoto Shinya <shinya34892@gmail.com> wrote:

> I ordered the valid encodings alphabetically and extracted those into separate parameters.

LGTM.  Per project style I don't think we should quote the names of encodings
since they are visibly not English words (see commit a243569bf65c5 which deals
with variable names, but I think it applies here as well), but no need to send
another version of the patch just for that small adjustment.

--
Daniel Gustafsson

On Mon, Nov 17, 2025 at 11:30 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 17 Nov 2025, at 15:08, Sugamoto Shinya <shinya34892@gmail.com> wrote:
>
> > I ordered the valid encodings alphabetically and extracted those into separate parameters.
>
> LGTM.  Per project style I don't think we should quote the names of encodings
> since they are visibly not English words (see commit a243569bf65c5 which deals
> with variable names, but I think it applies here as well)

It looks like the message-style rule added in commit a243569bf65c5
applies only to
configuration parameter names, right? And it seems that rule was later revised
by commit 17974ec2594. Is that correct?

Regards,

--
Fujii Masao

> On 17 Nov 2025, at 15:59, Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Mon, Nov 17, 2025 at 11:30 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>>
>>> On 17 Nov 2025, at 15:08, Sugamoto Shinya <shinya34892@gmail.com> wrote:
>>
>>> I ordered the valid encodings alphabetically and extracted those into separate parameters.
>>
>> LGTM.  Per project style I don't think we should quote the names of encodings
>> since they are visibly not English words (see commit a243569bf65c5 which deals
>> with variable names, but I think it applies here as well)
>
> It looks like the message-style rule added in commit a243569bf65c5
> applies only to
> configuration parameter names, right? And it seems that rule was later revised
> by commit 17974ec2594. Is that correct?

Oh right, I had forgotten about that, thanks for the reminder.  The patch as
proposed is correct then.

--
Daniel Gustafsson

On Mon, Nov 17, 2025 at 04:04:17PM +0100, Daniel Gustafsson wrote:
> Oh right, I had forgotten about that, thanks for the reminder.  The patch as
> proposed is correct then.

The patch is probably fine as-is, but this seems like a candidate for the
ClosestMatch stuff added by commit 5ac51c8.

-- 
nathan

On Mon, Nov 17, 2025 at 8:44 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Mon, Nov 17, 2025 at 04:04:17PM +0100, Daniel Gustafsson wrote:
> > Oh right, I had forgotten about that, thanks for the reminder.  The patch as
> > proposed is correct then.
>
> The patch is probably fine as-is, but this seems like a candidate for the
> ClosestMatch stuff added by commit 5ac51c8.

+1

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Tue, Nov 18, 2025 at 5:34 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Nov 17, 2025 at 8:44 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
> >
> > On Mon, Nov 17, 2025 at 04:04:17PM +0100, Daniel Gustafsson wrote:
> > > Oh right, I had forgotten about that, thanks for the reminder.  The patch as
> > > proposed is correct then.
> >
> > The patch is probably fine as-is, but this seems like a candidate for the
> > ClosestMatch stuff added by commit 5ac51c8.
>
> +1

Is there a guideline on when we should use the ClosestMatch machinery
instead of explicitly listing valid values? From the discussion in [1],
it seems appropriate when there are many possible values.
But should we generally replace all hints that list valid values
with ClosestMatch, or only in specific cases?

Regards,

[1] https://www.postgresql.org/message-id/flat/b1f9f399-3a1a-b554-283f-4ae7f34608e2@enterprisedb.com

--
Fujii Masao

On Wed, Nov 19, 2025 at 12:20:34AM +0900, Sugamoto Shinya wrote:
> On Tue, Nov 18, 2025 at 1:07 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>> Is there a guideline on when we should use the ClosestMatch machinery
>> instead of explicitly listing valid values? From the discussion in [1],
>> it seems appropriate when there are many possible values.
>> But should we generally replace all hints that list valid values
>> with ClosestMatch, or only in specific cases?

I think it makes sense to reserve it for cases with several possible
values.

> Personally I agree with this. Since there are only the four options in
> this case, it wouldn't be difficult for users to find a correct encoding
> from the hint message even without showing a possible encoding.  Also,
> using ClosestMatch here to show a possible encoding would encourage
> similar additions later which might bloat our error handling. Keeping
> code simple by notifying the user of the valid encodings should be
> sufficient for this part.

WFM

-- 
nathan

On Wed, Nov 19, 2025 at 8:08 PM Sugamoto Shinya <shinya34892@gmail.com> wrote:
>
> Thanks everyone for reviewing my proposal. Please let me know if you have something to discuss here.

+                                errhint("Valid encodings are \"%s\",
\"%s\", \"%s\", and \"%s\".",
+                                                "base64",
"base64url", "escape", "hex")));

You changed the HINT message to use format arguments,
but I still think it's better to embed the values directly (e.g.,
Valid encodings are "base64", "base64url", "escape", and "hex".)
for consistency with similar HINT messages. If this message is
used in many places a parameterized style would help translators,
but that benefit doesn't apply here.

Attached is an updated version of the patch. I switched
the HINT message to the embedded form and updated the commit message.
Barring any objections, I'm thinking to commit this.

Regards,

--
Fujii Masao

> On 19 Nov 2025, at 14:01, Fujii Masao <masao.fujii@gmail.com> wrote:

> You changed the HINT message to use format arguments,
> but I still think it's better to embed the values directly (e.g.,
> Valid encodings are "base64", "base64url", "escape", and "hex".)
> for consistency with similar HINT messages. If this message is
> used in many places a parameterized style would help translators,
> but that benefit doesn't apply here.
> 
> Attached is an updated version of the patch. I switched
> the HINT message to the embedded form and updated the commit message.
> Barring any objections, I'm thinking to commit this.

My intepreration was that this version was already objected to by Peter
upthread in c5c937dc-de8e-4284-be25-5d5eaf089d00@eisentraut.org.  The point
there being that punctuation in lists is subject to translation.

--
Daniel Gustafsson

On Wed, Nov 19, 2025 at 10:04 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 19 Nov 2025, at 14:01, Fujii Masao <masao.fujii@gmail.com> wrote:
>
> > You changed the HINT message to use format arguments,
> > but I still think it's better to embed the values directly (e.g.,
> > Valid encodings are "base64", "base64url", "escape", and "hex".)
> > for consistency with similar HINT messages. If this message is
> > used in many places a parameterized style would help translators,
> > but that benefit doesn't apply here.
> >
> > Attached is an updated version of the patch. I switched
> > the HINT message to the embedded form and updated the commit message.
> > Barring any objections, I'm thinking to commit this.
>
> My intepreration was that this version was already objected to by Peter
> upthread in c5c937dc-de8e-4284-be25-5d5eaf089d00@eisentraut.org.  The point
> there being that punctuation in lists is subject to translation.

Yes, so I was thinking a message like "Valid encodings are %s" isn't acceptable
since the punctuation included in the %s value is not translated. In contrast,
a message like "Valid encodings are \"base64\", …" is fine because all
punctuation
is part of the translatable string itself, I thought.

But maybe I'm missing something?

Regards,

--
Fujii Masao

On 19.11.25 14:47, Fujii Masao wrote:
> On Wed, Nov 19, 2025 at 10:04 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>>
>>> On 19 Nov 2025, at 14:01, Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>>> You changed the HINT message to use format arguments,
>>> but I still think it's better to embed the values directly (e.g.,
>>> Valid encodings are "base64", "base64url", "escape", and "hex".)
>>> for consistency with similar HINT messages. If this message is
>>> used in many places a parameterized style would help translators,
>>> but that benefit doesn't apply here.
>>>
>>> Attached is an updated version of the patch. I switched
>>> the HINT message to the embedded form and updated the commit message.
>>> Barring any objections, I'm thinking to commit this.
>>
>> My intepreration was that this version was already objected to by Peter
>> upthread in c5c937dc-de8e-4284-be25-5d5eaf089d00@eisentraut.org.  The point
>> there being that punctuation in lists is subject to translation.
> 
> Yes, so I was thinking a message like "Valid encodings are %s" isn't acceptable
> since the punctuation included in the %s value is not translated. In contrast,
> a message like "Valid encodings are \"base64\", …" is fine because all
> punctuation
> is part of the translatable string itself, I thought.

My point was a different one.  It is generally preferable to separate 
translatable and untranslated things.  So, as an example from elsewhere, 
instead of

errhint("Use ALTER DOMAIN instead.");

it would be better to use

errhint("Use % instead.", "ALTER DOMAIN");

Similarly, here I would prefer something along the lines of

errhint("Valid values are \"%s\", \"%s\", and \"%s\".", "foo", "bar", 
"baz");

So like it was done in patch v3 looks good to me.

On Wed, Nov 19, 2025 at 10:57 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> My point was a different one.  It is generally preferable to separate
> translatable and untranslated things.  So, as an example from elsewhere,
> instead of
>
> errhint("Use ALTER DOMAIN instead.");
>
> it would be better to use
>
> errhint("Use % instead.", "ALTER DOMAIN");
>
> Similarly, here I would prefer something along the lines of
>
> errhint("Valid values are \"%s\", \"%s\", and \"%s\".", "foo", "bar",
> "baz");
>
> So like it was done in patch v3 looks good to me.

Thanks for the explanation! I misunderstood your point.

So I will commit v3 patch.

Regards,

--
Fujii Masao

> On 19 Nov 2025, at 15:17, Fujii Masao <masao.fujii@gmail.com> wrote:

> So I will commit v3 patch.

+1

--
Daniel Gustafsson

On Wed, Nov 19, 2025 at 11:19 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 19 Nov 2025, at 15:17, Fujii Masao <masao.fujii@gmail.com> wrote:
>
> > So I will commit v3 patch.
>
> +1

I've pushed the patch. Thanks!

Regards,

--
Fujii Masao