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 <[email protected]> wrote:
>
>
> > On Nov 10, 2025, at 22:43, Daniel Gustafsson <[email protected]> wrote:
> >
> >> On 10 Nov 2025, at 10:06, Chao Li <[email protected]> wrote:
> >>> On Nov 8, 2025, at 14:25, Sugamoto Shinya <[email protected]>
> 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/
>
>
>
>
>
