Обсуждение: refactor backend type lists

Hello

We have lists of backend types scattered through the tree.  I found two
current ones, and Euler Taveira wants to add a couple more[1].  His
patch is actually blocked on not adding more, so this seems worth doing.
Bikeshedding welcome (for a limited time).

[1] https://postgr.es/m/313aa202-b5fa-4e3f-95d0-83425575c66d@app.fastmail.com

A couple points.  First, launch_backend.c has its own strings for
process names, inconsistent with the ones in miscinit.c.  I think we
should ignore the ones in launch_backend.c, because they're less
polished and not used for anything interesting, whereas the ones in
miscinit.c::GetBackendTypeDesc() are -- particularly init_ps_display and
pg_stat_activity.

Second, in discussion [2] leading to commit 18d67a8d7d30 (Nov 2024) it
was agreed to add support for translating backend type descriptions.
I'm not really sure that this is useful.  It would be, if set_ps_display
and pg_stat_activity used translated names, so that you could match what
log messages say with what the process lists show.  But I think we've
historically not translated those.  We have a few translatable strings
as the argument of HandleChildCrash() in postmaster.c, but those are
using names that are yet a third source of strings; I'm not a fan of
this.  (For staters, if a translation decided to use non-ascii chars for
process names, would that work okay in set_ps_display?  I bet it
wouldn't, because that's using strlen()).  So I would propose to rewind
a bit here, and remove translation from all those places so that the
output is consistent (== usable) between log messages and ps/pg_stat_activity.

[2] https://www.postgresql.org/message-id/flat/a102f15f-eac4-4ff2-af02-f9ff209ec66f%40iki.fi


Third, I didn't do it here, but HandleChildCrash is currently called
like
                HandleChildCrash(pid, exitstatus,
                                 _("WAL writer process"));
creating yet another source of strings to describe process types.

I think it would be much better to avoid that by using
                HandleChildCrash(pid, exitstatus,
                                 _(GetBackendTypeDesc(B_WAL_WRITER));

instead.  If we decide to get rid of the translation, then
                HandleChildCrash(pid, exitstatus,
                                 GetBackendTypeDesc(B_WAL_WRITER);

This last one would be my preference actually.  Note that we use this
string in an error that looks like this (LogChildExit):

        /*------
          translator: %s is a noun phrase describing a child process, such as
          "server process" */
                (errmsg("%s (PID %d) exited with exit code %d",
                        procname, pid, WEXITSTATUS(exitstatus)),

I would change this to
errmsg("process %d of type \"%s\" exited with exit code %d",
       pid, procname, WEXITSTATUS())
Note that in the original, the process name is translated, and in my
proposal it wouldn't be.  (This helps match the log message with "ps"
and pg_stat_activity).


Fourth: patch 0002 is a necessary hack to get the proctype_list.h
strings be picked up for translation by gettext in makefiles.  It's
quite ugly!  I'd rather not have it at all.  I have no idea how to do
this in Meson.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"I dream about dreams about dreams", sang the nightingale
under the pale moon (Sandman)

On Tue, Jul 15, 2025, at 3:30 PM, Álvaro Herrera wrote:
>
> We have lists of backend types scattered through the tree.  I found two
> current ones, and Euler Taveira wants to add a couple more[1].  His
> patch is actually blocked on not adding more, so this seems worth doing.
> Bikeshedding welcome (for a limited time).
>

I was trying to find a general solution for Andres feedback related to backend
types. It seems your proposal is good enough for future development in this
area.

> A couple points.  First, launch_backend.c has its own strings for
> process names, inconsistent with the ones in miscinit.c.  I think we
> should ignore the ones in launch_backend.c, because they're less
> polished and not used for anything interesting, whereas the ones in
> miscinit.c::GetBackendTypeDesc() are -- particularly init_ps_display and
> pg_stat_activity.
>

Agree.

> Second, in discussion [2] leading to commit 18d67a8d7d30 (Nov 2024) it
> was agreed to add support for translating backend type descriptions.
> I'm not really sure that this is useful.  It would be, if set_ps_display
> and pg_stat_activity used translated names, so that you could match what
> log messages say with what the process lists show.  But I think we've
> historically not translated those.  We have a few translatable strings
> as the argument of HandleChildCrash() in postmaster.c, but those are
> using names that are yet a third source of strings; I'm not a fan of
> this.  (For staters, if a translation decided to use non-ascii chars for
> process names, would that work okay in set_ps_display?  I bet it
> wouldn't, because that's using strlen()).  So I would propose to rewind
> a bit here, and remove translation from all those places so that the
> output is consistent (== usable) between log messages and ps/pg_stat_activity.
>

I'm not sure if it is a good idea to have translated backend description. The
init_ps_display() output is certainly used to obtain information (PID?) from a
process list. There is also the option %b from log_line_prefix that is used to
filter log messages per backend type. The same applies to backend_type column
from pg_stat_activity view.

> Third, I didn't do it here, but HandleChildCrash is currently called
> like
>                 HandleChildCrash(pid, exitstatus,
>                                  _("WAL writer process"));
> creating yet another source of strings to describe process types.
>
> I think it would be much better to avoid that by using
>                 HandleChildCrash(pid, exitstatus,
>                                  _(GetBackendTypeDesc(B_WAL_WRITER));
>
> instead.  If we decide to get rid of the translation, then
>                 HandleChildCrash(pid, exitstatus,
>                                  GetBackendTypeDesc(B_WAL_WRITER);
>

GetBackendTypeDesc() is a good use for this case. Looking at the
HandleChildCrash() function, I don't like the translated string (3rd argument)
with the "process" word too.

> This last one would be my preference actually.  Note that we use this
> string in an error that looks like this (LogChildExit):
>
>         /*------
>           translator: %s is a noun phrase describing a child process, such as
>           "server process" */
>                 (errmsg("%s (PID %d) exited with exit code %d",
>                         procname, pid, WEXITSTATUS(exitstatus)),
>
> I would change this to
> errmsg("process %d of type \"%s\" exited with exit code %d",
>        pid, procname, WEXITSTATUS())
> Note that in the original, the process name is translated, and in my
> proposal it wouldn't be.  (This helps match the log message with "ps"
> and pg_stat_activity).
>

+1. It might break translation. Your proposal is much better.

>
> Fourth: patch 0002 is a necessary hack to get the proctype_list.h
> strings be picked up for translation by gettext in makefiles.  It's
> quite ugly!  I'd rather not have it at all.  I have no idea how to do
> this in Meson.
>

We generally don't translated the other PG_XXX lists. I wouldn't do for the
reasons mentioned above. Let's have a stable list of backend types that we can
filter.

I'm attaching complementary patch that implements what you proposed above. I
also suggest that you rename the new file from proctype_list.h to
proctypelist.h. The other similar files don't use underscore.


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

On 2025-Jul-28, Euler Taveira wrote:

> On Tue, Jul 15, 2025, at 3:30 PM, Álvaro Herrera wrote:

> > Second, in discussion [2] leading to commit 18d67a8d7d30 (Nov 2024) it
> > was agreed to add support for translating backend type descriptions.
> > I'm not really sure that this is useful.  It would be, if set_ps_display
> > and pg_stat_activity used translated names, so that you could match what
> > log messages say with what the process lists show.  But I think we've
> > historically not translated those.  We have a few translatable strings
> > as the argument of HandleChildCrash() in postmaster.c, but those are
> > using names that are yet a third source of strings; I'm not a fan of
> > this.  (For staters, if a translation decided to use non-ascii chars for
> > process names, would that work okay in set_ps_display?  I bet it
> > wouldn't, because that's using strlen()).  So I would propose to rewind
> > a bit here, and remove translation from all those places so that the
> > output is consistent (== usable) between log messages and ps/pg_stat_activity.
> 
> I'm not sure if it is a good idea to have translated backend description. The
> init_ps_display() output is certainly used to obtain information (PID?) from a
> process list. There is also the option %b from log_line_prefix that is used to
> filter log messages per backend type. The same applies to backend_type column
> from pg_stat_activity view.

I should have let you know that I spent some time on this today as well
to avoid duplicating efforts.  Here are my patches, incorporating your
fixup -- I hadn't looked at your 0004 yet, so I wrote it differently,
passing the BackendType enum directly to LogChildExit (as well as
HandleChildCrash), so it is the former function that does the call to
GetBackendTypeDesc() to obtain the string.  I think this is better
because the untranslated part of the sentence is enclosed in quotes,
which I think is better.  However it meant I had to add a bit of a hack
to cope with the background worker separate bgw_type string.

So I came up with things as attached, incorporating your 0003 fixup.
What do you think?


This is likely not final, because the lines for background workers look
a bit inconsistent:

2025-07-28 23:10:02.316 CEST worker_spi dynamic[1876557] FATAL:  terminating background worker "worker_spi dynamic" due
toadministrator command
 
2025-07-28 23:10:02.317 CEST postmaster[1876543] LOG:  "background worker" process of type "logical replication
launcher"(PID 1876552) exited with exit code 1
 


(I, for one, would be VERY HAPPY to not have to translate the phrase
"background worker".  There's just no reasonable way.)

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On Mon, Jul 28, 2025, at 6:13 PM, Álvaro Herrera wrote:
> I should have let you know that I spent some time on this today as well
> to avoid duplicating efforts.  Here are my patches, incorporating your
> fixup -- I hadn't looked at your 0004 yet, so I wrote it differently,
> passing the BackendType enum directly to LogChildExit (as well as
> HandleChildCrash), so it is the former function that does the call to
> GetBackendTypeDesc() to obtain the string.  I think this is better
> because the untranslated part of the sentence is enclosed in quotes,
> which I think is better.  However it meant I had to add a bit of a hack
> to cope with the background worker separate bgw_type string.
>
> So I came up with things as attached, incorporating your 0003 fixup.
> What do you think?
>
Passing BackendType to HandleChildCrash() and LogChildExit() is a good idea.
Someone might forget to add GetBackendTypeDesc(B_XXX) in the future.

Talking about the messages, I think we should have a clean message. IMO
"process of type" seems confusing at first glance. My suggestion is to use:

checkpointer process (PID %d) ...

and

background worker process "foo" (PID %d) ...

or a short variant

background worker "foo" (PID %d) ...

You added quotes to say it is an untranslated part of the sentence but the
current message doesn't have quotes. The background worker name (addtype)
should have quotes because it is a different string depending on the module.
However, the proctype is a stable and well known string. Isn't it better to
improve the translator command saying "don't translate the proctype" instead of
adding quotes?

Another suggestion is to rename "addtype" variable. The substring "add" is
ambiguous when you want to meant "additional". Although you replaced "procname"
with "proctype", I think "procname" can be a good fit for this variable.

>
> This is likely not final, because the lines for background workers look
> a bit inconsistent:
>
> 2025-07-28 23:10:02.316 CEST worker_spi dynamic[1876557] FATAL:
> terminating background worker "worker_spi dynamic" due to administrator
> command
> 2025-07-28 23:10:02.317 CEST postmaster[1876543] LOG:  "background
> worker" process of type "logical replication launcher" (PID 1876552)
> exited with exit code 1
>

It is not related to this patch but this message is annoying and confusing. I
expect in the future we have a notion of in-core background worker so we can
differentiate exit code and omit this message for logical replication (and any
in-core feature that uses background worker). There is a patch from Thomas that
tries to improve it [1].

>
> (I, for one, would be VERY HAPPY to not have to translate the phrase
> "background worker".  There's just no reasonable way.)
>
+1. As I said the process type is a stable and well known string that we don't
expect to change.


[1] https://www.postgresql.org/message-id/CAEepm%3D10MtmKeDc1WxBM0PQM9OgtNy%2BRCeWqz40pZRRS3PNo5Q%40mail.gmail.com


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

On Thu, Sep 18, 2025 at 11:33 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:

> > I'm not sure if it is a good idea to have translated backend description. The
> > init_ps_display() output is certainly used to obtain information (PID?) from a
> > process list. There is also the option %b from log_line_prefix that is used to
> > filter log messages per backend type. The same applies to backend_type column
> > from pg_stat_activity view.
>
> I should have let you know that I spent some time on this today as well
> to avoid duplicating efforts.  Here are my patches, incorporating your
> fixup -- I hadn't looked at your 0004 yet, so I wrote it differently,
> passing the BackendType enum directly to LogChildExit (as well as
> HandleChildCrash), so it is the former function that does the call to
> GetBackendTypeDesc() to obtain the string.  I think this is better
> because the untranslated part of the sentence is enclosed in quotes,
> which I think is better.  However it meant I had to add a bit of a hack
> to cope with the background worker separate bgw_type string.
>
> So I came up with things as attached, incorporating your 0003 fixup.
> What do you think?
>

The call to gettext_noop() was missing, hence, the translation for the
backends was
missing. I've attached a third commit to your patch adding the missing
gettext_noop()
and also adding the include directory to the nls.mk Makefile which
will add proctypelist.h
to the list of files used to generate the po.

I've tested the patch using `make -C src/backend update-po` and the
strings were added
properly.

Regards,

On 2025-Sep-18, Jonathan Abdiel Gonzalez Valdebenito wrote:

> The call to gettext_noop() was missing, hence, the translation for the
> backends was missing. I've attached a third commit to your patch
> adding the missing gettext_noop() and also adding the include
> directory to the nls.mk Makefile which will add proctypelist.h to the
> list of files used to generate the po.

Thank you, that makes sense.  CI says that we still have problems in
mingw32 cross-compile apparently because of 0002, so I decided to ditch
it for now (it's non-essential anyway).  Another failure is `make
headerscheck`, which hopefully I've fixed her: just tell it to skip this
file.  Here's v4, which I would push right away except I want to see
if CI now turns green.

We can pursue 0002 later, if anybody feels the motivation to do so.  I
think it'd be a good idea.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Ed is the standard text editor."
      http://groups.google.com/group/alt.religion.emacs/msg/8d94ddab6a9b0ad3

On 2025-Sep-26, Álvaro Herrera wrote:

> Thank you, that makes sense.  CI says that we still have problems in
> mingw32 cross-compile apparently because of 0002, so I decided to ditch
> it for now (it's non-essential anyway).  Another failure is `make
> headerscheck`, which hopefully I've fixed her: just tell it to skip this
> file.  Here's v4, which I would push right away except I want to see
> if CI now turns green.

Pushed now, thanks.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/