Feedback-ID: ia2694551:Fastmail
Date: Wed, 5 Feb 2025 19:51:20 +0100
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@alvh.no-ip.org>
To: Euler Taveira <euler@eulerto.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: log_min_messages per backend type
Message-ID: <202502051851.2nvzh3a6ukzc@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <e85c6671-1600-4112-8887-f97a8a5d07b2@app.fastmail.com>
Archived-At: <https://www.postgresql.org/message-id/202502051851.2nvzh3a6ukzc%40alvherre.pgsql>
Precedence: bulk

Hello Euler,

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';


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 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.

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.


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.


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>.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"No necesitamos banderas
 No reconocemos fronteras"                  (Jorge González)