> On Dec 10, 2025, at 02:24, Alvaro Herrera <[email protected]> wrote:
>
> On 2025-Dec-09, Alvaro Herrera wrote:
>
>> So here's your v6 again with those fixes as 0003 -- let's see what CI
>> thinks of this. I haven't looked at your doc changes yet.
>
> This passed CI, so I have marked it as Ready for Committer. Further
> comments are still welcome, of course, but if there are none, I intend
> to get it committed in a few days.
>
>
> I'm not really happy with our usage of the translatable description
> field for things such as %b in log_line_prefix or the ps display; I
> think we should use the shorter names there instead. Otherwise, you end
> up with log lines that look something like this (visible in a server
> with %b in log_line_prefix, which the TAP tests as well as pg_regress
> have):
>
> 2025-12-08 21:38:04.304 CET autovacuum launcher[2452437] DEBUG: autovacuum
> launcher started
>
> where the bit before the PID is marked for translation. I think it
> should rather be
>
> 2025-12-08 21:38:04.304 CET autovacuum[2452437] DEBUG: autovacuum launcher
> started
>
> where that name (the same we'll use in log_min_messages) is not
> translated.
>
> However, this issue is rather independent of the patch in this thread,
> so I'm going to discuss it in another thread; the name string though is
> added by this patch.
>
> --
> Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
> "Java is clearly an example of money oriented programming" (A. Stepanov)
Overall the patch is solid, I just have a couple of suggestions:
1 - user experience
```
evantest=# show log_min_messages;
log_min_messages
---------------------
warning,backend:log
(1 row)
```
Now “show log_min_messages” prints the raw string the user set, in above
example, there is not a white-space between the two log levels, and “show”
result doesn’t have a white-space between the two log levels either. IMO, “SHOW
log_min_messages” should display a stable result, in other words, say “fatal,
backend:log” and “backend:log, fatal” should show the same result as they are
actually meaning the same. So, I would suggest normalize the raw string: put
the general level in the first place and sort others by process type, then SHOW
returns the normalized string.
2 - refactoring for 0001
```
+ sep = strchr(tok, ':');
+ if (sep == NULL)
+ {
+ bool found = false;
+
+ /* Reject duplicates for generic log level. */
+ if (genericlevel != -1)
+ {
+ GUC_check_errdetail("Generic log level was
already assigned.");
+ guc_free(rawstring);
+ list_free(elemlist);
+ return false;
+ }
+
+ /* Is the log level valid? */
+ for (entry = server_message_level_options; entry &&
entry->name; entry++)
+ {
+ if (pg_strcasecmp(entry->name, tok) == 0)
+ {
+ genericlevel = entry->val;
+ found = true;
+ break;
+ }
+ }
+
+ if (!found)
+ {
+ GUC_check_errdetail("Unrecognized log level:
\"%s\".", tok);
+ guc_free(rawstring);
+ list_free(elemlist);
+ return false;
+ }
+ }
+ else
+ {
+ char *loglevel;
+ char *ptype;
+ bool found = false;
+
+ ptype = guc_malloc(LOG, (sep - tok) + 1);
+ if (!ptype)
+ {
+ guc_free(rawstring);
+ list_free(elemlist);
+ return false;
+ }
+ memcpy(ptype, tok, sep - tok);
+ ptype[sep - tok] = '\0';
+ loglevel = sep + 1;
+
+ /* Is the log level valid? */
+ for (entry = server_message_level_options; entry &&
entry->name; entry++)
+ {
+ if (pg_strcasecmp(entry->name, loglevel) == 0)
+ {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found)
+ {
+ GUC_check_errdetail("Unrecognized log level:
\"%s\".", loglevel);
+ guc_free(ptype);
+ guc_free(rawstring);
+ list_free(elemlist);
+ return false;
+ }
```
In the “if” and “else” clauses, there are duplicate code to valid log levels.
We should refactor the code to avoid the duplication. For example, pull up
“loglevel” to the “for” loop level, then we can valid it after the “if-else”.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
