| From: | Tan Yang <332696245(at)qq(dot)com> |
|---|---|
| To: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Euler Taveira <euler(at)eulerto(dot)com> |
| Cc: | japin <japinli(at)hotmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, pgsql-hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, li(dot)evan(dot)chao <li(dot)evan(dot)chao(at)gmail(dot)com> |
| Subject: | Re: log_min_messages per backend type |
| Date: | 2025-12-10 03:34:38 |
| Message-ID: | tencent_3A00C6E675F356FC8EA915E4C670ABDAD609@qq.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi,
I applied this patch, and I have a few thoughts.
=== Issue 1: Unhelpful error messages for typos ===
When users make typos in process types or log levels, the current error
messages don't provide helpful hints. For example:
```
testdb=# SET log_min_messages TO 'autovacum:debug1, warning';
ERROR: invalid value for parameter "log_min_messages": "autovacum:debug1, warning"
DETAIL: Unrecognized process type: "autovacum".
testdb=# SET log_min_messages TO 'autovacuum:debug1, warnong';
ERROR: invalid value for parameter "log_min_messages": "autovacuum:debug1, warnong"
DETAIL: Unrecognized log level: "warnong".
```
typed in a wrong process type or log level, I got stuck.
Where to find the right values? We can add HINT, to list all candidate values.
I have seen a similar change that lists all valid values in HINT. See [1].
=== Issue 2: Confusing error message for duplicate generic settings ===
The error message for duplicate generic log levels isn't immediately clear:
```
testdb=# SET log_min_messages TO 'debug1, backend:error, fatal';
ERROR: invalid value for parameter "log_min_messages": "debug1, backend:error, fatal"
DETAIL: Generic log level was already assigned.
```
This DETAIL message is unclear. It leads to a misunderstanding that a previous SET had assigned the generic log level.
I think "Only one default log level may be specified.” would be better.
=== Issue 3: Minor optimization for ptype allocation ===
In the following code snippet:
```
+ ptype = guc_malloc(LOG, (sep - tok) + 1);
+ if (!ptype)
+ {
+ guc_free(rawstring);
+ list_free(elemlist);
+ return false;
+ }
```
"ptype" is a temp buffer to store parsed process type, I think it can be allocated from stack. Because the valid values are not long
we could use like:
char ptype[MAXNAMELEN];
This would eliminate the need for allocation failure handling and simplify
memory management, as we wouldn't need to worry about freeing it before
every return path.
------------------ 原始邮件 ------------------
发件人: "Alvaro Herrera" <alvherre(at)alvh(dot)no-ip(dot)org>;
发送时间: 2025年12月10日(星期三) 凌晨2:24
收件人: "Euler Taveira"<euler(at)eulerto(dot)com>;
抄送: "japin"<japinli(at)hotmail(dot)com>;"Andres Freund"<andres(at)anarazel(dot)de>;"pgsql-hackers"<pgsql-hackers(at)lists(dot)postgresql(dot)org>;"Chao Li"<li(dot)evan(dot)chao(at)gmail(dot)com>;
主题: Re: log_min_messages per backend type
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)
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Tatsuya Kawata | 2025-12-10 03:44:19 | Re: [PATCH] Add memory usage reporting to VACUUM VERBOSE |
| Previous Message | Zhijie Hou (Fujitsu) | 2025-12-10 03:32:39 | RE: Assertion failure in SnapBuildInitialSnapshot() |