public inbox for [email protected]
help / color / mirror / Atom feedFrom: Euler Taveira <[email protected]>
To: Chao Li <[email protected]>
Cc: japin <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: [email protected]
Subject: Re: log_min_messages per backend type
Date: Tue, 25 Nov 2025 23:15:21 -0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<ME0P300MB04459538922993E1BD0DC051B627A@ME0P300MB0445.AUSP300.PROD.OUTLOOK.COM>
<[email protected]>
<[email protected]>
<[email protected]>
On Thu, Nov 6, 2025, at 11:53 AM, Chao Li wrote:
> I just reviewed the patch and got a few comments.
>
Thanks for your review.
> + /* Initialize the array. */
> + memset(newlevel, WARNING, BACKEND_NUM_TYPES * sizeof(int));
> ```
>
> I think this statement is wrong, because memset() writes bytes but
> integers, so this statement will set very byte to WARNING, which should
> not be the intention. You will need to use a loop to initialize every
> element of newlevel.
>
Facepalm. Good catch.
> 2 - variable.c
> ```
> + /* Parse string into list of identifiers. */
> + if (!SplitGUCList(rawstring, ',', &elemlist))
> + {
> + /* syntax error in list */
> + GUC_check_errdetail("List syntax is invalid.");
> + guc_free(rawstring);
> + list_free(elemlist);
> + return false;
> + }
> ```
>
> Every element of elemlist points to a position of rawstring, so it’s
> better to list_free(elemlist) first then guc_free(rawstring).
>
Fixed.
> 3 - launch_backend.c
> ```
> static inline bool
> should_output_to_server(int elevel)
> {
> - return is_log_level_output(elevel, log_min_messages);
> + return is_log_level_output(elevel, log_min_messages[MyBackendType]);
> }
> ```
>
> Is it possible that when this function is called, MyBackendType has not
> been initialized? To be safe, maybe we can check if MyBackendType is 0
> (B_INVALID), then use the generic log_min_message.
>
It uses the generic log level if you don't modify backend type "backend". If
you do specify "backend", that level is used instead. After Alvaro's question
in the other email, I added "postmaster" backend type and assigned it to
B_INVALID. That's exactly the log level that will be used. As I said in that
email, it is ok to consider a process whose backend type is B_INVALID as a
postmaster process.
> * “Valid values are …”, here “are” usually mean both are needed, so
> maybe change “are” to “can be”.
> * It says “the single level is mandatory”, then why there is still a
> default value?
>
It seems the current sentence that describe the comma-separated list is
ambiguous. The sentence doesn't make it clear that the list contains 2 elements
(type:level and level) and the "level" element is mandatory. What about the new
sentence?
--
Euler Taveira
EDB https://www.enterprisedb.com/
view thread (20+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: log_min_messages per backend type
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox