> On Feb 6, 2026, at 23:45, Euler Taveira <[email protected]> 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 convenient way 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 into the 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 could in theory merge with the existing
setting), the current ALTER SYSTEM path doesn’t preserve the transformed value
returned by the check hook. In AlterSystemSetConfigFile(), the newval produced
by parse_and_validate_value() is effectively discarded 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 could easily do:
```
# set log_min_messages = debug5
```
to temporarily increase verbosity for the current backend. With the new syntax,
doing the equivalent now requires copying the 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 merging semantics or some other mechanism)?
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
