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/