Re: [PATCH] Add hints for invalid binary encoding names in encode/decode functions

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

// 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 benefit to 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 string would 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?

Regards,

On Tue, Nov 11, 2025 at 3:14 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:

>
>
> > On Nov 10, 2025, at 22:43, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> >
> >> On 10 Nov 2025, at 10:06, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
> >>> On Nov 8, 2025, at 14:25, Sugamoto Shinya <shinya34892(at)gmail(dot)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 with a 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 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 benefit to hardcoding 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/
>
>
>
>
>