Обсуждение: Re: log_min_messages per backend type
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)
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 otherway around, "backendtype:loglevel" ... that seems more natural becauseit'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 inlaunch_backend.c (child_process_kinds), plus GetBackendTypeDesc inmiscinit.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 withexceptions for particular backend types:log_min_messages = WARNING, autovacuum:DEBUG1Right now I think the code only accepts the unadorned log level if thereare no other items in the list. I think the proposal downthread is touse the keyword ALL for this,log_min_messages = all:WARNING, autovacuum:DEBUG1 # I don't like thisbut I think it's inferior, because then "all" is not really "all", and Ithink it would be different if I had saidlog_min_messages = autovacuum:DEBUG1, all:WARNING # I don't like thisbecause it looks like the "all" entry should override the one I set forautovacuum 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:DEBUG1log_min_messages = autovacuum:DEBUG1, WARNINGshould behave identically and mean "set the level for autovacuum toDEBUG1, and to any other backend type to WARNING.
Done.
Also, I think it'd be better to reject duplicates in the list. Rightnow it looks like the last entry for one backend type overrides priorones. I meanlog_min_messages = autovacuum:DEBUG1, autovacuum:ERRORwould set autovacuum to error, but that might be mistake prone if yourstring is long. So implementation-wise I suggest to initialize thewhole newlogminmsgs array to -1, then scan the list of entries (savingan entry without backend type as the one to use later and) setting everybackend type to the number specified; if we see trying to set a valuethat's already different from -1, throw error. After scanning the wholelog_min_messages array, we scan the newlogminmsgs and set any entriesthat are still -1 to the value that we saved before.The new code in variable.c should be before the /* DATESTYLE */ commentrather 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 thevalid values for backendtype, as well as show an example of a validsetting. Please don't use ALLCAPS backend types in the docs, this looksugly:> + 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
Вложения
On 2025-03-04 Tu 7:33 PM, Euler Taveira wrote:
I think it should be acceptable to configure one global setting withexceptions for particular backend types:log_min_messages = WARNING, autovacuum:DEBUG1Right now I think the code only accepts the unadorned log level if thereare no other items in the list. I think the proposal downthread is touse the keyword ALL for this,log_min_messages = all:WARNING, autovacuum:DEBUG1 # I don't like thisbut I think it's inferior, because then "all" is not really "all", and Ithink it would be different if I had saidlog_min_messages = autovacuum:DEBUG1, all:WARNING # I don't like thisbecause it looks like the "all" entry should override the one I set forautovacuum 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:DEBUG1log_min_messages = autovacuum:DEBUG1, WARNINGshould behave identically and mean "set the level for autovacuum toDEBUG1, and to any other backend type to WARNING.
Done.
Just bikeshedding a bit ...
I'm not mad keen on this design. I think the value should be either a single setting like "WARNING" or a set of type:setting pairs. I agree that "all" is a bad name, but I think "default" would make sense.
I can live with it but I think this just looks a bit odd.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On 2025/03/05 9:33, Euler Taveira wrote: >> > + 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>. What about postmaster? For parallel workers launched for parallel queries, should they follow the backend's log level or the background worker's? Since they operate as part of a parallel query executed by a backend, it seems more logical for them to follow the backend's setting. + [B_CHECKPOINTER] = "checkpointer", + [B_STARTUP] = "backend", /* XXX same as backend? */ I like the idea of allowing log levels to be set per process. There were times I wanted to use debug5 specifically for the startup process when troubleshooting WAL replay. It would be helpful to distinguish the startup process from a regular backend, so we can set its log level independently. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Hi, > > On 2025-03-04 Tu 7:33 PM, Euler Taveira wrote: >>> 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. > > > Just bikeshedding a bit ... > > I'm not mad keen on this design. I think the value should be either a > single setting like "WARNING" or a set of type:setting pairs. I agree > that "all" is a bad name, but I think "default" would make sense. > > I can live with it but I think this just looks a bit odd. > Just bringing some thoughts about it... How about using something like *:WARNING? I'm not sure if it could also be confusing as the "all" keyword, but I think it could also be interpreted as "anything else use WARNING". -- Matheus Alcantara
Hi,
On 2025-03-04 21:33:39 -0300, Euler Taveira wrote:
> +/*
> + * This must match enum BackendType! It should be static, but
> + * commands/variable.c needs to get at this.
> + */
> +int log_min_messages[] = {
> + [B_INVALID] = WARNING,
> + [B_BACKEND] = WARNING,
> + [B_DEAD_END_BACKEND] = WARNING,
> + [B_AUTOVAC_LAUNCHER] = WARNING,
> + [B_AUTOVAC_WORKER] = WARNING,
> + [B_BG_WORKER] = WARNING,
> + [B_WAL_SENDER] = WARNING,
> + [B_SLOTSYNC_WORKER] = WARNING,
> + [B_STANDALONE_BACKEND] = WARNING,
> + [B_ARCHIVER] = WARNING,
> + [B_BG_WRITER] = WARNING,
> + [B_CHECKPOINTER] = WARNING,
> + [B_STARTUP] = WARNING,
> + [B_WAL_RECEIVER] = WARNING,
> + [B_WAL_SUMMARIZER] = WARNING,
> + [B_WAL_WRITER] = WARNING,
> + [B_LOGGER] = WARNING,
> +};
> +StaticAssertDecl(lengthof(log_min_messages) == BACKEND_NUM_TYPES,
> + "array length mismatch");
> +
> +/*
> + * This must match enum BackendType! It might be in commands/variable.c but for
> + * convenience it is near log_min_messages.
> + */
> +const char *const log_min_messages_backend_types[] = {
> + [B_INVALID] = "backend", /* XXX same as backend? */
> + [B_BACKEND] = "backend",
> + [B_DEAD_END_BACKEND] = "backend", /* XXX same as backend? */
> + [B_AUTOVAC_LAUNCHER] = "autovacuum",
> + [B_AUTOVAC_WORKER] = "autovacuum",
> + [B_BG_WORKER] = "bgworker",
> + [B_WAL_SENDER] = "walsender",
> + [B_SLOTSYNC_WORKER] = "slotsyncworker",
> + [B_STANDALONE_BACKEND] = "backend", /* XXX same as backend? */
> + [B_ARCHIVER] = "archiver",
> + [B_BG_WRITER] = "bgwriter",
> + [B_CHECKPOINTER] = "checkpointer",
> + [B_STARTUP] = "backend", /* XXX same as backend? */
Huh, the startup process is among the most crucial things to monitor?
> + [B_WAL_RECEIVER] = "walreceiver",
> + [B_WAL_SUMMARIZER] = "walsummarizer",
> + [B_WAL_WRITER] = "walwriter",
> + [B_LOGGER] = "logger",
> +};
> +
> +StaticAssertDecl(lengthof(log_min_messages_backend_types) == BACKEND_NUM_TYPES,
> + "array length mismatch");
> +
I don't know what I think about the whole patch, but I do want to voice
*strong* opposition to duplicating a list of all backend types into multiple
places. It's already painfull enough to add a new backend type, without having
to pointlessly go around and manually add a new backend type to mulltiple
arrays that have completely predictable content.
Greetings,
Andres Freund
On Wed, Mar 5, 2025, at 11:53 PM, Fujii Masao wrote:
On 2025/03/05 9:33, Euler Taveira wrote:>> > + 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>.What about postmaster?
It is B_INVALID that is currently mapped to "backend". This state is used as an
initial phase for the child process. There might be messages before
MyBackendType is assigned (see ProcessStartupPacket, for example). Hence, I
refrain to create a special backend type for postmaster. Should we map it to
generic log level instead of backend?
For parallel workers launched for parallel queries, should they followthe backend's log level or the background worker's? Since they operateas part of a parallel query executed by a backend, it seems more logicalfor them to follow the backend's setting.
Since we are using enum BackendType and there is no special entry for parallel
query processes. I'm afraid that introducing conditional logic for checking
special cases like the bgw_type for parallel queries or MyProcPid ==
PostmasterPid might slowdown that code path. (See that should_output_to_server
is an inline function.) I will run some tests to see if there is a considerable
impact.
+ [B_CHECKPOINTER] = "checkpointer",+ [B_STARTUP] = "backend", /* XXX same as backend? */I like the idea of allowing log levels to be set per process.There were times I wanted to use debug5 specifically forthe startup process when troubleshooting WAL replay. It would behelpful to distinguish the startup process from a regular backend,so we can set its log level independently.
Although startup process is mapped to backend (hence, the XXX), we can
certainly create a separate backend type for it.
On Wed, Mar 5, 2025, at 1:40 PM, Andrew Dunstan wrote:
Just bikeshedding a bit ...
I'm not mad keen on this design. I think the value should be either a single setting like "WARNING" or a set of type:setting pairs. I agree that "all" is a bad name, but I think "default" would make sense.
One of my main concerns was a clear interface. I think "default" is less
confusing than "all". Your suggestion about single or list is aligned with what
Alvaro suggested. IIUC you are suggesting default:loglevel only if it is part
of the list; the single loglevel shouldn't contain the backend type to keep the
backward compatibility. The advantage of your proposal is that it make it clear
what the fallback log level is. However, someone could be confused asking if
the "default" is a valid backend type and if there is a difference between
WARNING and default:WARNING (both is a fallback for non-specified backend type
elements).
On Thu, Mar 6, 2025, at 10:33 AM, Andres Freund wrote: > Huh, the startup process is among the most crucial things to monitor? > Good point. Fixed. After collecting some suggestions, I'm attaching a new patch contains the following changes: - patch was rebased - include Alvaro's patch (v2-0001) [1] as a basis for this patch - add ioworker as new backend type - add startup as new backend type per Andres suggestion - small changes into documentation > I don't know what I think about the whole patch, but I do want to voice > *strong* opposition to duplicating a list of all backend types into multiple > places. It's already painfull enough to add a new backend type, without having > to pointlessly go around and manually add a new backend type to mulltiple > arrays that have completely predictable content. > I'm including Alvaro's patch as is just to make the CF bot happy and to illustrate how it would be if we adopt his solution to centralize the list of backend types. I think Alvaro's proposal overcomes the objection [2], right? [1] https://www.postgresql.org/message-id/202507282113.vdp4axehoppi@alvherre.pgsql [2] https://www.postgresql.org/message-id/y5tgui75jrcj6mm5nmoq4yqwage2432akx4kp2ogtcnim3wskx@2ipmtfi4qvpi -- Euler Taveira EDB https://www.enterprisedb.com/
Вложения
On Thu, 31 Jul 2025 at 11:19, "Euler Taveira" <euler@eulerto.com> wrote: > On Thu, Mar 6, 2025, at 10:33 AM, Andres Freund wrote: >> Huh, the startup process is among the most crucial things to monitor? >> > > Good point. Fixed. > > After collecting some suggestions, I'm attaching a new patch contains the > following changes: > > - patch was rebased > - include Alvaro's patch (v2-0001) [1] as a basis for this patch > - add ioworker as new backend type > - add startup as new backend type per Andres suggestion > - small changes into documentation > >> I don't know what I think about the whole patch, but I do want to voice >> *strong* opposition to duplicating a list of all backend types into multiple >> places. It's already painfull enough to add a new backend type, without having >> to pointlessly go around and manually add a new backend type to mulltiple >> arrays that have completely predictable content. >> > > I'm including Alvaro's patch as is just to make the CF bot happy and to > illustrate how it would be if we adopt his solution to centralize the list of > backend types. I think Alvaro's proposal overcomes the objection [2], right? > If we set the log level for all backend types, I don't think a generic log level is necessary. -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
On 2025-Aug-01, Japin Li wrote: > If we set the log level for all backend types, I don't think a generic log > level is necessary. I don't understand what you mean by this. Can you elaborate? -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "If it is not right, do not do it. If it is not true, do not say it." (Marcus Aurelius, Meditations)
On Thu, 31 Jul 2025 at 18:51, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > On 2025-Aug-01, Japin Li wrote: > >> If we set the log level for all backend types, I don't think a generic log >> level is necessary. > > I don't understand what you mean by this. Can you elaborate? > For example: ALTER SYSTEM SET log_min_messages TO 'archiver:info, autovacuum:info, backend:info, bgworker:info, bgwriter:info, checkpointer:info, ioworker:info, logger:info,slotsyncworker:info, startup:info, walreceiver:info, walsender:info, walsummarizer:info, walwriter:info'; Given that we've set a log level for every backend type and assigned[BACKEND_NUM_TYPES] is true, a generic level seems redundant. -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
On Thu, Jul 31, 2025 at 11:19:48AM -0300, Euler Taveira wrote:
> On Thu, Mar 6, 2025, at 10:33 AM, Andres Freund wrote:
> > Huh, the startup process is among the most crucial things to monitor?
> >
>
> Good point. Fixed.
>
> After collecting some suggestions, I'm attaching a new patch contains the
> following changes:
>
> - patch was rebased
> - include Alvaro's patch (v2-0001) [1] as a basis for this patch
> - add ioworker as new backend type
> - add startup as new backend type per Andres suggestion
> - small changes into documentation
>
> > I don't know what I think about the whole patch, but I do want to voice
> > *strong* opposition to duplicating a list of all backend types into multiple
> > places. It's already painfull enough to add a new backend type, without having
> > to pointlessly go around and manually add a new backend type to mulltiple
> > arrays that have completely predictable content.
> >
>
> I'm including Alvaro's patch as is just to make the CF bot happy and to
> illustrate how it would be if we adopt his solution to centralize the list of
> backend types. I think Alvaro's proposal overcomes the objection [2], right?
>
I think we can avoid memory allocation by using pg_strncasecmp().
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 5c769dd7bcc..f854b2fac93 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -1343,14 +1343,10 @@ check_log_min_messages(char **newval, void **extra, GucSource source)
}
else
{
- char *loglevel;
- char *btype;
- bool found = false;
- btype = palloc((sep - tok) + 1);
- memcpy(btype, tok, sep - tok);
- btype[sep - tok] = '\0';
- loglevel = pstrdup(sep + 1);
+ char *btype = tok;
+ char *loglevel = sep + 1;
+ bool found = false;
/* Is the log level valid? */
for (entry = server_message_level_options; entry && entry->name; entry++)
@@ -1377,7 +1373,7 @@ check_log_min_messages(char **newval, void **extra, GucSource source)
found = false;
for (int i = 0; i < BACKEND_NUM_TYPES; i++)
{
- if (pg_strcasecmp(log_min_messages_backend_types[i], btype) == 0)
+ if (pg_strncasecmp(log_min_messages_backend_types[i], btype, sep - tok) == 0)
{
/* Reject duplicates for a backend type. */
if (assigned[i])
--
Best regards,
Japin Li
ChengDu WenWu Information Technology Co., LTD.
On Thu, Jul 31, 2025, at 8:34 PM, Japin Li wrote: > On Thu, 31 Jul 2025 at 18:51, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: >> On 2025-Aug-01, Japin Li wrote: >> >>> If we set the log level for all backend types, I don't think a generic log >>> level is necessary. >> >> I don't understand what you mean by this. Can you elaborate? >> > For example: > > ALTER SYSTEM SET log_min_messages TO > 'archiver:info, autovacuum:info, backend:info, bgworker:info, > bgwriter:info, checkpointer:info, ioworker:info, logger:info, > slotsyncworker:info, startup:info, walreceiver:info, walsender:info, > walsummarizer:info, walwriter:info'; > > Given that we've set a log level for every backend type and > assigned[BACKEND_NUM_TYPES] is true, a generic level seems redundant. > The main reason I didn't consider this idea was that this GUC assignment will fail as soon as a new backend type is added. Restore a dump should work across major versions. > I think we can avoid memory allocation by using pg_strncasecmp(). > loglevel does not required a memory allocation but I didn't include the btype part because it complicates the error message that uses btype a few lines above. This new patch contains the following changes: - patch was rebased - use commit dbf8cfb4f02 - use some GUC memory allocation functions - avoid one memory allocation (suggested by Japin Li) - rename backend type: logger -> syslogger - adjust tests to increase test coverage - improve documentation and comments to reflect the current state -- Euler Taveira EDB https://www.enterprisedb.com/
Вложения
On Sun, Oct 5, 2025, at 11:18 AM, Euler Taveira wrote: > This new patch contains the following changes: > > - patch was rebased > - use commit dbf8cfb4f02 > - use some GUC memory allocation functions > - avoid one memory allocation (suggested by Japin Li) > - rename backend type: logger -> syslogger > - adjust tests to increase test coverage > - improve documentation and comments to reflect the current state > Patch was rebased since commit fce7c73fba4 broke it. No modifications. -- Euler Taveira EDB https://www.enterprisedb.com/
Вложения
Hi Euler, I just reviewed the patch and got a few comments. > On Nov 6, 2025, at 21:09, Euler Taveira <euler@eulerto.com> wrote: > > On Sun, Oct 5, 2025, at 11:18 AM, Euler Taveira wrote: >> This new patch contains the following changes: >> >> - patch was rebased >> - use commit dbf8cfb4f02 >> - use some GUC memory allocation functions >> - avoid one memory allocation (suggested by Japin Li) >> - rename backend type: logger -> syslogger >> - adjust tests to increase test coverage >> - improve documentation and comments to reflect the current state >> > > Patch was rebased since commit fce7c73fba4 broke it. No modifications. > > > -- > Euler Taveira > EDB https://www.enterprisedb.com/<v5-0001-log_min_messages-per-backend-type.patch> 1 - variable.c ``` + /* 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. 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). 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 checkif MyBackendType is 0 (B_INVALID), then use the generic log_min_message. 4 - config.sgml ``` + Valid values are a comma-separated list of <literal>backendtype:level</literal> + and a single <literal>level</literal>. The list allows it to use + different levels per backend type. Only the single <literal>level</literal> + is mandatory (order does not matter) and it is assigned to the backend + types that are not specified in the list. + 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>ioworker</literal>, + <literal>syslogger</literal>, <literal>slotsyncworker</literal>, + <literal>startup</literal>, <literal>walreceiver</literal>, + <literal>walsender</literal>, <literal>walsummarizer</literal>, and + <literal>walwriter</literal>. + Valid <literal>level</literal> values are <literal>DEBUG5</literal>, + <literal>DEBUG4</literal>, <literal>DEBUG3</literal>, <literal>DEBUG2</literal>, + <literal>DEBUG1</literal>, <literal>INFO</literal>, <literal>NOTICE</literal>, + <literal>WARNING</literal>, <literal>ERROR</literal>, <literal>LOG</literal>, + <literal>FATAL</literal>, and <literal>PANIC</literal>. Each level includes + all the levels that follow it. The later the level, the fewer messages are sent + to the log. The default is <literal>WARNING</literal> for all backend types. + Note that <literal>LOG</literal> has a different rank here than in ``` * “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? Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
On 2025-Nov-06, Euler Taveira wrote: > + Valid values are a comma-separated list of <literal>backendtype:level</literal> > + and a single <literal>level</literal>. The list allows it to use > + different levels per backend type. Only the single <literal>level</literal> > + is mandatory (order does not matter) and it is assigned to the backend > + types that are not specified in the list. > + 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>ioworker</literal>, > + <literal>syslogger</literal>, <literal>slotsyncworker</literal>, > + <literal>startup</literal>, <literal>walreceiver</literal>, > + <literal>walsender</literal>, <literal>walsummarizer</literal>, and > + <literal>walwriter</literal>. > + Valid <literal>level</literal> values are <literal>DEBUG5</literal>, > + <literal>DEBUG4</literal>, <literal>DEBUG3</literal>, <literal>DEBUG2</literal>, > + <literal>DEBUG1</literal>, <literal>INFO</literal>, <literal>NOTICE</literal>, > + <literal>WARNING</literal>, <literal>ERROR</literal>, <literal>LOG</literal>, > + <literal>FATAL</literal>, and <literal>PANIC</literal>. Each level includes > + all the levels that follow it. The later the level, the fewer messages are sent > + to the log. The default is <literal>WARNING</literal> for all backend types. I would use <literal>type:level</literal> rather than "backendtype". Also, the glossary says we have "auxiliary processes" and "backends", so from the user perspective, these types are not all backends, but instead process types. I think the list of backend types is pretty hard to read. What do you think about using <simplelist type="vert" columns="4"> to create a friendlier representation? So you would say something like "Valid types are listed in the table below, each corresponding to either postmaster, an auxiliary process type or a backend". (Eh, wait, you seem not to have "postmaster" in your list, what's up with that?) (I'm not sure about making the log levels also a <simplelist>, because for that list the order matters, and if we have more than one column then the order is going to be ambiguous, and if we have just one column it's going to be too tall. But maybe it's not all that bad?) -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
On Thu, Nov 6, 2025, at 1:01 PM, Alvaro Herrera wrote:
>
> I would use <literal>type:level</literal> rather than "backendtype".
> Also, the glossary says we have "auxiliary processes" and "backends", so
> from the user perspective, these types are not all backends, but instead
> process types.
>
Hasn't that ship already sailed? If your suggestion is limited to "type:level",
that's ok. I wouldn't like to use 2 terminologies ("process type" and "backend
type") in the documentation.
See miscadmin.h.
/*
* MyBackendType indicates what kind of a backend this is.
*
* If you add entries, please also update the child_process_kinds array in
* launch_backend.c.
*/
typedef enum BackendType
The "backend type" terminology is exposed. It is even available in the
pg_stat_activity view. I agree that "backend" is not a good name to define a
Postgres process. I don't think we should be inconsistent only in this patch.
Even if the proposal is to rename enum BackendType (I won't propose it as part
of this patch), it would make some extension authors unhappy according to
codesearch.debian.net.
> I think the list of backend types is pretty hard to read. What do you
> think about using
> <simplelist type="vert" columns="4">
> to create a friendlier representation?
>
I tried but didn't like the result. See the attached image. You said table but
it is a multi-column list; it doesn't contain a header or borders. Reading this
message and Chao Li message, I realized the description is not good enough yet.
> So you would say something like "Valid types are listed in the table
> below, each corresponding to either postmaster, an auxiliary process
> type or a backend". (Eh, wait, you seem not to have "postmaster" in
> your list, what's up with that?)
>
Good question. The current patch uses "backend" to B_INVALID (that's exactly the
MyBackendType for postmaster -- see below). I think it is reasonable to create a
new category "postmaster" and assign it to B_INVALID. If, for some reason, a
process temporarily has MyBackendType equals to B_INVALID, it is ok to still
consider it a postmaster process.
$ ps aux | grep postgres
euler 56968 0.0 0.0 217144 29016 ? Ss 19:28 0:00 /c/pg/bin/postgres -D /c/pg/pgdata
euler 56969 0.0 0.0 217276 9948 ? Ss 19:28 0:00 postgres: io worker 0
euler 56970 0.0 0.0 217276 7620 ? Ss 19:28 0:00 postgres: io worker 1
euler 56971 0.0 0.0 217144 6408 ? Ss 19:28 0:00 postgres: io worker 2
euler 56972 0.0 0.0 217276 8388 ? Ss 19:28 0:00 postgres: checkpointer
euler 56973 0.0 0.0 217300 8744 ? Ss 19:28 0:00 postgres: background writer
euler 56975 0.0 0.0 217276 11292 ? Ss 19:28 0:00 postgres: walwriter
euler 56976 0.0 0.0 218724 9140 ? Ss 19:28 0:00 postgres: autovacuum launcher
euler 56977 0.0 0.0 218724 9232 ? Ss 19:28 0:00 postgres: logical replication launcher
euler 58717 0.0 0.0 6676 2232 pts/1 S+ 19:39 0:00 grep --color=auto postgres
$ for p in $(pgrep postgres); do echo "pid: $p" && gdb -q -batch -ex 'set pagination off' -ex "attach $p" -ex 'p
MyBackendType'-ex 'quit' 2> /dev/null; done | grep -E '^\$1|pid'
pid: 56968
$1 = B_INVALID
pid: 56969
$1 = B_IO_WORKER
pid: 56970
$1 = B_IO_WORKER
pid: 56971
$1 = B_IO_WORKER
pid: 56972
$1 = B_CHECKPOINTER
pid: 56973
$1 = B_BG_WRITER
pid: 56975
$1 = B_WAL_WRITER
pid: 56976
$1 = B_AUTOVAC_LAUNCHER
pid: 56977
$1 = B_BG_WORKER
> (I'm not sure about making the log levels also a <simplelist>, because
> for that list the order matters, and if we have more than one column
> then the order is going to be ambiguous, and if we have just one column
> it's going to be too tall. But maybe it's not all that bad?)
>
+1.
--
Euler Taveira
EDB https://www.enterprisedb.com/
Вложения
On 2025-Nov-18, Euler Taveira wrote: > On Thu, Nov 6, 2025, at 1:01 PM, Alvaro Herrera wrote: > See miscadmin.h. > > /* > * MyBackendType indicates what kind of a backend this is. > * > * If you add entries, please also update the child_process_kinds array in > * launch_backend.c. > */ > typedef enum BackendType > > The "backend type" terminology is exposed. Something appearing in the source code does not equate it being exposed to end users. Things are exposed when they are in the documentation, or when the SQL interface requires you to use the term. So I don't think the fact that BackendType appears in the code forces us to use that name in the user-visible interface now. In the glossary, we talk about "processes"; in the definitions there, "backend" is one type of process. So in this list of "backend types" that you want to introduce, what you really should be talking about is "process types", where "backend" is one of the options. > It is even available in the > pg_stat_activity view. I agree that "backend" is not a good name to define a > Postgres process. I don't think we should be inconsistent only in this patch. > Even if the proposal is to rename enum BackendType (I won't propose it as part > of this patch), it would make some extension authors unhappy according to > codesearch.debian.net. I don't think we should rename the BackendType enum. That's in the source code, not in the documentation or the SQL interface, so we don't need to care. > > I think the list of backend types is pretty hard to read. What do you > > think about using > > <simplelist type="vert" columns="4"> > > to create a friendlier representation? > > I tried but didn't like the result. See the attached image. Well, I like it very much. The original is a solid wall of text, very hard to read, where you have to squint hunting commas in order to distinguish one item from the next. (Maybe you are young and have good eyesight, but you won't be young forever.) In the screenshot you show, the list of possible process types to use is nicely separated, which makes it very easy to catch at a glance. Maybe remove "the table", which is obviously inappropriate, and just say "Valid process types are listed below, each corresponding to either postmaster, an auxiliary process type or a backend". (Note your original wording says "a backend type, corresponding to [blah blah] or a backend" which makes no sense -- how is a backend a type of backend? It isn't. It's a type of process.) > You said table but it is a multi-column list; it doesn't contain a > header or borders. Right. You don't need a full-blown table here: this simple list is perfectly adequate. > Good question. The current patch uses "backend" to B_INVALID (that's exactly the > MyBackendType for postmaster -- see below). I think it is reasonable to create a > new category "postmaster" and assign it to B_INVALID. I guess that would work, but I think it's inadequate. Maybe we could add a new value B_POSTMASTER and have postmaster switch to that as early as possible. Then anything that still has B_INVALID must necessarily be an improperly identified process. Users wouldn't assign a value to that one (the GUC wouldn't let you); instead those would always use the default value. Hopefully nobody would see that very often, or at all. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
On Wed, Nov 19, 2025, at 7:44 AM, Alvaro Herrera wrote:
> On 2025-Nov-18, Euler Taveira wrote:
>> The "backend type" terminology is exposed.
>
> Something appearing in the source code does not equate it being exposed
> to end users. Things are exposed when they are in the documentation, or
> when the SQL interface requires you to use the term. So I don't think
> the fact that BackendType appears in the code forces us to use that name
> in the user-visible interface now.
>
It is *already* exposed.
$ grep -B 3 -A 3 'backend type' doc/src/sgml/config.sgml
</informaltable>
<para>
The backend type corresponds to the column
<structfield>backend_type</structfield> in the view
<link linkend="monitoring-pg-stat-activity-view">
<structname>pg_stat_activity</structname></link>,
--
character count of the error position therein,
location of the error in the PostgreSQL source code
(if <varname>log_error_verbosity</varname> is set to <literal>verbose</literal>),
application name, backend type, process ID of parallel group leader,
and query id.
Here is a sample table definition for storing CSV-format log output:
$ grep -B 3 -A 3 'backend_type' doc/src/sgml/config.sgml
<para>
The backend type corresponds to the column
<structfield>backend_type</structfield> in the view
<link linkend="monitoring-pg-stat-activity-view">
<structname>pg_stat_activity</structname></link>,
but additional types can appear
--
query_pos integer,
location text,
application_name text,
backend_type text,
leader_pid integer,
query_id bigint,
PRIMARY KEY (session_id, session_line_num)
--
<entry>Client application name</entry>
</row>
<row>
<entry><literal>backend_type</literal></entry>
<entry>string</entry>
<entry>Type of backend</entry>
</row>
$ psql -c "SELECT backend_type FROM pg_stat_activity" -d postgres
backend_type
------------------------------
client backend
autovacuum launcher
logical replication launcher
io worker
io worker
io worker
checkpointer
background writer
walwriter
(9 rows)
> In the glossary, we talk about "processes"; in the definitions there,
> "backend" is one type of process. So in this list of "backend types"
> that you want to introduce, what you really should be talking about is
> "process types", where "backend" is one of the options.
>
>> It is even available in the
>> pg_stat_activity view. I agree that "backend" is not a good name to define a
>> Postgres process. I don't think we should be inconsistent only in this patch.
>> Even if the proposal is to rename enum BackendType (I won't propose it as part
>> of this patch), it would make some extension authors unhappy according to
>> codesearch.debian.net.
>
> I don't think we should rename the BackendType enum. That's in the
> source code, not in the documentation or the SQL interface, so we don't
> need to care.
>
I think you missed my point here. As I said if your proposal will let us with 2
terminologies (backend type and process type) as shown above. We can debate if
we (1) keep the current terminology (backend type), (2) use the proposed one
(process type) or (3) use a mixed variation (keep the SQL interface --
backend_type column but change the description to "process type"). I wouldn't
like to break compatibility (pg_stat_activity changes tend to break a lot of
stuff) so I'm fine with (1) and (3).
>> > I think the list of backend types is pretty hard to read. What do you
>> > think about using
>> > <simplelist type="vert" columns="4">
>> > to create a friendlier representation?
>>
>> I tried but didn't like the result. See the attached image.
>
> Well, I like it very much. The original is a solid wall of text, very
> hard to read, where you have to squint hunting commas in order to
> distinguish one item from the next. (Maybe you are young and have good
> eyesight, but you won't be young forever.) In the screenshot you show,
> the list of possible process types to use is nicely separated, which
> makes it very easy to catch at a glance. Maybe remove "the table",
> which is obviously inappropriate, and just say "Valid process types are
> listed below, each corresponding to either postmaster, an auxiliary
> process type or a backend".
>
I don't have a strong preference. One advantage of this suggestion is that it
is visually easier to identify the types.
> (Note your original wording says "a backend type, corresponding to [blah
> blah] or a backend" which makes no sense -- how is a backend a type of
> backend? It isn't. It's a type of process.)
>
I'm rewriting that paragraph.
>> Good question. The current patch uses "backend" to B_INVALID (that's exactly the
>> MyBackendType for postmaster -- see below). I think it is reasonable to create a
>> new category "postmaster" and assign it to B_INVALID.
>
> I guess that would work, but I think it's inadequate. Maybe we could
> add a new value B_POSTMASTER and have postmaster switch to that as early
> as possible. Then anything that still has B_INVALID must necessarily be
> an improperly identified process. Users wouldn't assign a value to that
> one (the GUC wouldn't let you); instead those would always use the
> default value. Hopefully nobody would see that very often, or at all.
>
I will create a separate patch for this suggestion.
--
Euler Taveira
EDB https://www.enterprisedb.com/
On Fri, Nov 21, 2025, at 11:13 AM, Euler Taveira wrote: > I think you missed my point here. As I said if your proposal will let us with 2 > terminologies (backend type and process type) as shown above. We can debate if > we (1) keep the current terminology (backend type), (2) use the proposed one > (process type) or (3) use a mixed variation (keep the SQL interface -- > backend_type column but change the description to "process type"). I wouldn't > like to break compatibility (pg_stat_activity changes tend to break a lot of > stuff) so I'm fine with (1) and (3). > After digesting it for a couple of days, I decided to adopt the terminology in commit dbf8cfb4f02e (process type) just for this patch. This discussion about changing "backend type" terminology in other parts belongs to another patch. >>> Good question. The current patch uses "backend" to B_INVALID (that's exactly the >>> MyBackendType for postmaster -- see below). I think it is reasonable to create a >>> new category "postmaster" and assign it to B_INVALID. >> >> I guess that would work, but I think it's inadequate. Maybe we could >> add a new value B_POSTMASTER and have postmaster switch to that as early >> as possible. Then anything that still has B_INVALID must necessarily be >> an improperly identified process. Users wouldn't assign a value to that >> one (the GUC wouldn't let you); instead those would always use the >> default value. Hopefully nobody would see that very often, or at all. >> > > I will create a separate patch for this suggestion. > After reflection, add B_POSTMASTER into BackendType / proctypelist.h is not a good idea. These data structures were designed to represent postmaster children. The child_process_kinds array (launch_backend.c) is assembled with proctypelist.h which means a function like postmaster_child_launch() could use one of its elements (e.g. B_POSTMASTER) to start a new child; that's awkward. Do you envision any issues to use B_INVALID for postmaster? (I wrote 0002 to minimize the windows that each child process has B_INVALID.) This new version contains the following changes: - fix variable assignment (Chao Li) - fix memory release order (Chao Li) - change category for B_INVALID (backend -> postmaster) (Alvaro) - rewrite documentation (Chao Li, Alvaro) - rename backend type to process type (Alvaro) - new patch (0002) that assigns MyBackendType as earlier as possible -- Euler Taveira EDB https://www.enterprisedb.com/
Вложения
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/