Re: log_min_messages per backend type - Mailing list pgsql-hackers
| From | Chao Li |
|---|---|
| Subject | Re: log_min_messages per backend type |
| Date | |
| Msg-id | 827C26CA-D7C2-4AD9-AEC3-E1E76519453E@gmail.com Whole thread Raw |
| In response to | Re: log_min_messages per backend type ("Euler Taveira" <euler@eulerto.com>) |
| Responses |
Re: log_min_messages per backend type
Re: log_min_messages per backend type |
| List | pgsql-hackers |
> On Feb 6, 2026, at 23:45, Euler Taveira <euler@eulerto.com> wrote:
>
> On Fri, Feb 6, 2026, at 8:05 AM, Alvaro Herrera wrote:
>> Here are some fixups for the main patch. I include yours so that CI
>> will test this one.
>>
>
> All suggestions seem good to me.
>
>> * I'm not fond of the term "generic", so I changed it to "default",
>> which IMO makes more sense from the user point of view.
>>
>
> I'm fine with "default" instead of "generic". However, you forgot to replace
> "generic" in a few places. The attached patch fixes them all.
>
>> * There seemed to be too much guc_mallocking[*] going on; for instance for
>> ptype. I simplified that by just overwriting the : to a '\0', then we
>> can use the original string for pg_strcasecmp(). At the bottom of the
>> loop we restore the ':', so that the code below can reuse the original
>> values. This is okay because we have already mallocked a copy for
>> SplitGUCList.
>>
>
> Nice trick.
>
>> * There's no reason to add a WARNING to every process type in the
>> proctypelist.h table. We can just set the values to WARNING in the
>> log_min_messages initializer in guc_tables.c.
>>
>
> Ok.
>
>> * There's no reason AFAICS to have log_min_messages_process_types in
>> guc_tables.c; nobody else uses it. I made it a local array of char*
>> inside the check_log_min_messages function.
>>
>
> That's true.
>
>> * There's no need to initialize the newlevel[] array, since we're going
>> to assign values to every individual element at one point or another.
>> And for the assigned[] array, we don't need to set the value of each to
>> 'false' in a loop; we can just initialize to '{0}' and the compiler will
>> do the needful. With this we can remove the loop entirely.
>>
>
> Ok.
>
>> * "guc_free()+list_free()+return false" the whole time in the various
>> failure places across the loop was tiresome. I put them all in a single
>> place and jump there with a goto.
>>
>
> I tend to avoid goto. However, in this case, it seems it improves readability.
>
>> * in foreach() loops, tracking of a 'bool first' is unnecessary. You
>> can just compare foreach_current_index() with 0.
>>
>
> Didn't know about foreach_current_index.
>
>> * Rewrote docs and comments
>>
>
> Ok.
>
> I attached the same patches that you shared plus the s/generic/default/ that I
> said earlier for the sake of CI.
>
>
> --
> Euler Taveira
> EDB
https://www.enterprisedb.com/<v11-0001-log_min_messages-per-process-type.patch><v11-0002-review.patch><v11-0003-fixup-review.patch>
Hi Euler,
Thanks for updating the patch. I briefly played with v11 and overall it looks solid, but I ran into one usability
concern.
While it’s reasonable to configure a full comma-separated list in postgresql.conf, it seems there’s currently no
convenientway for a superuser to change the log level of a single process type. For example:
```
evantest=# alter system set log_min_messages='backend:debug4';
ERROR: invalid value for parameter "log_min_messages": "backend:debug4"
DETAIL: Default log level was not defined.
```
This feels a bit inconvenient in practice. Intuitively, it would be nice if setting a single type:level could be merged
intothe existing log_min_messages value, rather than requiring users to restate the entire configuration.
I dug into this a bit and noticed that, while check_log_min_messages() already normalizes and sorts the value (and
couldin theory merge with the existing setting), the current ALTER SYSTEM path doesn’t preserve the transformed value
returnedby the check hook. In AlterSystemSetConfigFile(), the newval produced by parse_and_validate_value() is
effectivelydiscarded for string GUCs:
```
if (!parse_and_validate_value(record, value,
PGC_S_FILE, ERROR,
&newval, &newextra))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid value for parameter \"%s\": \"%s\"",
name, value)));
if (record->vartype == PGC_STRING && newval.stringval != NULL)
guc_free(newval.stringval);
```
So even if the check hook tried to be smarter, the infrastructure doesn’t currently allow it.
There’s also a related regression in the interactive case. Before this patch, when debugging via psql, a superuser
couldeasily do:
```
# set log_min_messages = debug5
```
to temporarily increase verbosity for the current backend. With the new syntax, doing the equivalent now requires
copyingthe entire log_min_messages string and modifying just one part, which feels noticeably heavier for quick
debugging.
Do you think it would make sense to support setting a specific process type’s log level more directly (either via
mergingsemantics or some other mechanism)?
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
pgsql-hackers by date: