Re: log_min_messages per backend type

On Wed, Feb 5, 2025, at 3:51 PM, Álvaro Herrera wrote:
> On 2024-Dec-17, Euler Taveira wrote:
>
> > Sometimes you need to inspect some debug messages from autovacuum worker but
> > you cannot apply the same setting for backends (that could rapidly fill the log
> > file). This proposal aims to change log_min_messages to have different log
> > levels depending on which backend type the message comes from.
> >
> > The syntax was changed from enum to string and it accepts a list of elements.
> >
> > Instead of enum, it now accepts a comma-separated list of elements (string).
> > Each element is LOGLEVEL:BACKENDTYPE.
>
> This format seems unintuitive. I would have thought you do it the other
> way around, "backendtype:loglevel" ... that seems more natural because
> it's like assigning the 'loglevel' value to the 'backendtype' element.
> SET log_min_messages TO 'checkpointer:debug2, autovacuum:debug1';

Alvaro, thanks for your feedback. Your suggestion makes sense to me.

> I dislike the array of names in variable.c. We already have an array in
> launch_backend.c (child_process_kinds), plus GetBackendTypeDesc in
> miscinit.c. Maybe not for this patch to clean up though.

I thought about using child_process_kinds but two details made me give
up on the idea: (a) multiple names per backend type (for example,
B_BACKEND, B_DEAD_END_BACKEND, B_STANDALONE_BACKEND) and (b) spaces into
names. Maybe we should have a group into child_process_kinds but as you
said it seems material for another patch.

> I think it should be acceptable to configure one global setting with
> exceptions for particular backend types:
>
> log_min_messages = WARNING, autovacuum:DEBUG1
>
> Right now I think the code only accepts the unadorned log level if there
> are no other items in the list. I think the proposal downthread is to
> use the keyword ALL for this,
>
> log_min_messages = all:WARNING, autovacuum:DEBUG1 # I don't like this
>
> but I think it's inferior, because then "all" is not really "all", and I
> think it would be different if I had said
>
> log_min_messages = autovacuum:DEBUG1, all:WARNING # I don't like this
>
> because it looks like the "all" entry should override the one I set for
> autovacuum before, which frankly would not make sense to me.

Good point. After reflection, I agree that "all" is not a good keyword.
This patch turns backend type as optional so WARNING means apply this
log level as a final step to the backend types that are not specified in
the list.

> So I think these two lines,
>
> log_min_messages = WARNING, autovacuum:DEBUG1
> log_min_messages = autovacuum:DEBUG1, WARNING
>
> should behave identically and mean "set the level for autovacuum to
> DEBUG1, and to any other backend type to WARNING.

Done.

> Also, I think it'd be better to reject duplicates in the list. Right
> now it looks like the last entry for one backend type overrides prior
> ones. I mean
>
> log_min_messages = autovacuum:DEBUG1, autovacuum:ERROR
>
> would set autovacuum to error, but that might be mistake prone if your
> string is long. So implementation-wise I suggest to initialize the
> whole newlogminmsgs array to -1, then scan the list of entries (saving
> an entry without backend type as the one to use later and) setting every
> backend type to the number specified; if we see trying to set a value
> that's already different from -1, throw error. After scanning the whole
> log_min_messages array, we scan the newlogminmsgs and set any entries
> that are still -1 to the value that we saved before.
>
>
> The new code in variable.c should be before the /* DATESTYLE */ comment
> rather than at the end of the file.

It was added into the MISCELLANEOUS section.

>
> You still have many XXX comments. Also, postgresql.conf should list the
> valid values for backendtype, as well as show an example of a valid
> setting. Please don't use ALLCAPS backend types in the docs, this looks
> ugly:
>
> > + Valid <literal>BACKENDTYPE</literal> values are <literal>ARCHIVER</literal>,
> > + <literal>AUTOVACUUM</literal>, <literal>BACKEND</literal>,
> > + <literal>BGWORKER</literal>, <literal>BGWRITER</literal>,
> > + <literal>CHECKPOINTER</literal>, <literal>LOGGER</literal>,
> > + <literal>SLOTSYNCWORKER</literal>, <literal>WALRECEIVER</literal>,
> > + <literal>WALSENDER</literal>, <literal>WALSUMMARIZER</literal>, and
> > + <literal>WALWRITER</literal>.

Done.

Just to recap what was changed:

- patch was rebased
- backend type is optional and means all unspecified backend types
- generic log level (without backend type) is mandatory and the order it
ppears is not important (it is applied for the remaining backend types)
- fix Windows build
- new tests to cover the changes

--
Euler Taveira
EDB https://www.enterprisedb.com/