Feedback-ID: i0c21471d:Fastmail
MIME-Version: 1.0
Date: Tue, 25 Nov 2025 23:15:21 -0300
From: "Euler Taveira" <euler@eulerto.com>
To: "Chao Li" <li.evan.chao@gmail.com>
Cc: japin <japinli@hotmail.com>, "Alvaro Herrera" <alvherre@alvh.no-ip.org>,
 "Andres Freund" <andres@anarazel.de>, pgsql-hackers@lists.postgresql.org
Message-Id: <ba5372fc-368b-4e53-a9f7-e1e9c78f570f@app.fastmail.com>
In-Reply-To: <8AF30EAA-2E9F-4398-B4C6-24C4480CA869@gmail.com>
References: <202507311651.ahhz57cfk6lo@alvherre.pgsql>
 <ME0P300MB04459538922993E1BD0DC051B627A@ME0P300MB0445.AUSP300.PROD.OUTLOOK.COM>
 <24b9bfd6-7fba-4946-9b73-244934b89464@app.fastmail.com>
 <72e809ff-c842-4f98-93f1-26a3cb3d92d0@app.fastmail.com>
 <8AF30EAA-2E9F-4398-B4C6-24C4480CA869@gmail.com>
Subject: Re: log_min_messages per backend type
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/ba5372fc-368b-4e53-a9f7-e1e9c78f570f%40app.fastmail.com>
Precedence: bulk

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=20
> integers, so this statement will set very byte to WARNING, which shoul=
d=20
> not be the intention. You will need to use a loop to initialize every=20
> 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=E2=80=
=99s=20
> 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 no=
t=20
> been initialized? To be safe, maybe we can check if MyBackendType is 0=20
> (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 que=
stion
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.

> * =E2=80=9CValid values are =E2=80=A6=E2=80=9D, here =E2=80=9Care=E2=80=
=9D usually mean both are needed, so=20
> maybe change =E2=80=9Care=E2=80=9D to =E2=80=9Ccan be=E2=80=9D.
> * It says =E2=80=9Cthe single level is mandatory=E2=80=9D, then why th=
ere is still a=20
> 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 e=
lements
(type:level and level) and the "level" element is mandatory. What about =
the new
sentence?


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