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

On Tue, Nov 11, 2025 at 7:14 AM Sugamoto Shinya <shinya34892(at)gmail(dot)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 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?

+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