Re: [PATCH] Add hints for invalid binary encoding names in encode/decode functions
| От | Sugamoto Shinya |
|---|---|
| Тема | Re: [PATCH] Add hints for invalid binary encoding names in encode/decode functions |
| Дата | |
| Msg-id | CAAe3y+_ibmscW51FA05PtJ_-pz0PsmKvw-TpNsM5FkL_kC5PdA@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: [PATCH] Add hints for invalid binary encoding names in encode/decode functions (Chao Li <li.evan.chao@gmail.com>) |
| Ответы |
Re: [PATCH] Add hints for invalid binary encoding names in encode/decode functions
|
| Список | pgsql-hackers |
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.
Thoughts?
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\".")
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")
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")));
"(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.
>
>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.
>
> * 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.
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.evan.chao@gmail.com> wrote:
> 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 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/
В списке pgsql-hackers по дате отправления: