A few message wording/formatting cleanup patches

On 28 May 2026, at 05:16, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

Hello,

While translating error messages into Japanese

Thank you so much for doing this!

Some comments on a few of your patches:

0002: 0002-Use-double-quotes-in-message.patch

fe-protocol3.c has the following message containing backquotes:

libpq_append_conn_error(conn,
"server did not report the unsupported `_pq_.test_protocol_negotiation` parameter in its protocol negotiation message");

but that doesn't seem to match our usual style. Nearby messages use
double quotes instead.

Agreed. Shouldn't it also be adding the parameter as a %s to further match our style?

-  libpq_append_conn_error(conn, "server did not report the unsupported `_pq_.test_protocol_negotiation` parameter in its protocol negotiation message");
+  libpq_append_conn_error(conn, "server did not report the unsupported \"%s\" parameter in its protocol negotiation message", "_pq_.test_protocol_negotiation");

0003: 0003-Add-missing-period-to-HINT-message.patch

In be-secure-openssl.c, the following HINT message is missing a
trailing period:

errhint("If ssl_sni is enabled then add configuration to "%s", else "%s"",

My bad, will fix. Reading the hint it's also not as helpful as it could be for
a hint I think, perhaps this would be better?

-  errhint("If ssl_sni is enabled then add configuration to \"%s\", else \"%s\"",
+  errhint("If ssl_sni is enabled then add configuration to \"%s\", else configure SSL in \"%s\".",

0004: 0004-Use-singular-datachecksum-consistently-in-process-na.patch

In datachecksum_state.c, the launcher process is referred to in two
different ways: "datachecksum launcher" and "datachecksums
launcher". Considering the worker process name, I think the former is
probably the intended one, so this patch makes the naming consistent
accordingly.

That said, I can also imagine an interpretation where "datachecksums"
was chosen intentionally to refer to the checksum feature or checksum
set as a whole, so I'm not entirely sure whether this should be
considered a real issue or just a stylistic inconsistency.

Still, having both forms coexist seems somewhat error-prone in
practice, especially when typing or searching symbol names.

I am clearly biased, or Stockholm syndromed, but DataChecksumsXXX was chosen
deliberately since it affects the feature which is exposed with the GUC
data_checksums. Renaming it does not improve clarity IMHO. The singular form
"checksum" is used where it refers to a single entity, like the cluster state
which can only be a single value.

It would however be an improvement to rename the "datachecksum launcher|worker"
cases you found to "datachecksums" since they are user facing.

- * This creates the list of databases for the datachecksumsworker workers to
+ * This creates the list of databases for the datachecksum workers to

This comment refers to a worker process of the type datachecksumsworker, the
proposed change makes that less clear IMO. That said, the original wording
isn't great so maybe "datachecksumsworker process" is better?

--
Daniel Gustafsson

On Thu, May 28, 2026 at 10:19 AM Daniel Gustafsson <daniel@yesql.se> wrote:

On 28 May 2026, at 05:16, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

libpq_append_conn_error(conn,
"server did not report the unsupported `_pq_.test_protocol_negotiation` parameter in its protocol negotiation message");

but that doesn't seem to match our usual style. Nearby messages use
double quotes instead.

Agreed. Shouldn't it also be adding the parameter as a %s to further match our style?

-  libpq_append_conn_error(conn, "server did not report the unsupported `_pq_.test_protocol_negotiation` parameter in its protocol negotiation message");
+  libpq_append_conn_error(conn, "server did not report the unsupported \"%s\" parameter in its protocol negotiation message", "_pq_.test_protocol_negotiation");

+1, will fix.

Thanks!
--Jacob

On 5/28/26 19:19, Daniel Gustafsson wrote:

On 28 May 2026, at 05:16, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

...

0004: 0004-Use-singular-datachecksum-consistently-in-process-na.patch

In datachecksum_state.c, the launcher process is referred to in two
different ways: "datachecksum launcher" and "datachecksums
launcher". Considering the worker process name, I think the former is
probably the intended one, so this patch makes the naming consistent
accordingly.

That said, I can also imagine an interpretation where "datachecksums"
was chosen intentionally to refer to the checksum feature or checksum
set as a whole, so I'm not entirely sure whether this should be
considered a real issue or just a stylistic inconsistency.

Still, having both forms coexist seems somewhat error-prone in
practice, especially when typing or searching symbol names.

I am clearly biased, or Stockholm syndromed, but DataChecksumsXXX was chosen
deliberately since it affects the feature which is exposed with the GUC
data_checksums. Renaming it does not improve clarity IMHO. The singular form
"checksum" is used where it refers to a single entity, like the cluster state
which can only be a single value.

It would however be an improvement to rename the "datachecksum launcher|worker"
cases you found to "datachecksums" since they are user facing.

- * This creates the list of databases for the datachecksumsworker workers to
+ * This creates the list of databases for the datachecksum workers to

This comment refers to a worker process of the type datachecksumsworker, the
proposed change makes that less clear IMO. That said, the original wording
isn't great so maybe "datachecksumsworker process" is better?

My vote would be to use the plural "data checksums" almost everywhere,
except for the places that actually manipulates a single data checksum.
Because the feature is "data checksums" with GUC "data_checksums", we
manipulate all checksums in the cluster, and so on.

So it'd be "datachecksums_state.c" and "datachecksums launcher" (which
would make it more consistent with the existing injection points, named
"datachecksumsworker-launcher-..." etc.).

But this opinion comes from someone who accidentally named an internal
queueing library "queque" and had to live with that shame for years.

regards

--
Tomas Vondra

On Thu, May 28, 2026 at 10:23 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:

+1, will fix.

(Pushed.)

--Jacob

On 28 May 2026, at 21:04, Tomas Vondra <tomas@vondra.me> wrote:

On 5/28/26 19:19, Daniel Gustafsson wrote:

On 28 May 2026, at 05:16, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

...

0004: 0004-Use-singular-datachecksum-consistently-in-process-na.patch

In datachecksum_state.c, the launcher process is referred to in two
different ways: "datachecksum launcher" and "datachecksums
launcher". Considering the worker process name, I think the former is
probably the intended one, so this patch makes the naming consistent
accordingly.

That said, I can also imagine an interpretation where "datachecksums"
was chosen intentionally to refer to the checksum feature or checksum
set as a whole, so I'm not entirely sure whether this should be
considered a real issue or just a stylistic inconsistency.

Still, having both forms coexist seems somewhat error-prone in
practice, especially when typing or searching symbol names.

I am clearly biased, or Stockholm syndromed, but DataChecksumsXXX was chosen
deliberately since it affects the feature which is exposed with the GUC
data_checksums. Renaming it does not improve clarity IMHO. The singular form
"checksum" is used where it refers to a single entity, like the cluster state
which can only be a single value.

It would however be an improvement to rename the "datachecksum launcher|worker"
cases you found to "datachecksums" since they are user facing.

- * This creates the list of databases for the datachecksumsworker workers to
+ * This creates the list of databases for the datachecksum workers to

This comment refers to a worker process of the type datachecksumsworker, the
proposed change makes that less clear IMO. That said, the original wording
isn't great so maybe "datachecksumsworker process" is better?

My vote would be to use the plural "data checksums" almost everywhere,
except for the places that actually manipulates a single data checksum.
Because the feature is "data checksums" with GUC "data_checksums", we
manipulate all checksums in the cluster, and so on.

So it'd be "datachecksums_state.c" and "datachecksums launcher" (which
would make it more consistent with the existing injection points, named
"datachecksumsworker-launcher-..." etc.).

I've pushed the worker/launcher rename now. I held off on renaming the file
for now, I'm not entirely convinced that's an improvement.

--
Daniel Gustafsson

Hi Horiguchi-san,

On Thu, May 28, 2026 at 8:46 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

Hello,

While translating error messages into Japanese, I came across some
unusually formatted messages and messages that are difficult to
translate naturally into Japanese.

0001: 0001-Make-propgraph-object-descriptions-translatable.patch

getObjectDescription() currently builds the object name for
PropgraphElementRelationId incrementally with appendStringInfo(), but
that works poorly with Japanese because it effectively fixes the word
order. We probably need to construct the whole name from a single
format string instead.

Sorry for delay in review. Few comments
1. There are other places in the function where we use similar code.
Those places call initStringInfo just before getRelationDescription()
is called and then pfree() StringInfoData.data after it is added to
the object description.
2. Why do we want to capture the output of getObjectDescription() in a
StringInfo and then add it to the buffer? I think we can pass
getObjectDescription directly as an argument to appendStringInfo()
similar to the code for case AttrDefaultRelationId.

--
Best Wishes,
Ashutosh Bapat

Hello. Thank you for the comments.

At Mon, 1 Jun 2026 17:18:13 +0530, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote in

1. There are other places in the function where we use similar code.
Those places call initStringInfo just before getRelationDescription()
is called and then pfree() StringInfoData.data after it is added to
the object description.

That's a good point. I hadn't paid much attention to that because the
allocation is short-lived, but matching the surrounding code is
probably better. I've added pfree() in the attached patch.

2. Why do we want to capture the output of getObjectDescription() in a
StringInfo and then add it to the buffer? I think we can pass
getObjectDescription directly as an argument to appendStringInfo()
similar to the code for case AttrDefaultRelationId.

I considered returning strings from helper functions such as
getRelationDescription(), but that would affect a number of similar
helper functions and broaden the scope of the patch.

While working on this, I noticed that the string returned by
getObjectDescription() in the AttrDefaultRelationId case is not
pfree'd. Since that appears unrelated to this patch, I left it as-is.

I also dropped the assertions on the temporary description strings.
They were mainly there while developing the patch and don't seem
necessary in the final version.

Thanks!

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Jun 2, 2026 at 10:13 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

Hello. Thank you for the comments.

At Mon, 1 Jun 2026 17:18:13 +0530, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote in

1. There are other places in the function where we use similar code.
Those places call initStringInfo just before getRelationDescription()
is called and then pfree() StringInfoData.data after it is added to
the object description.

That's a good point. I hadn't paid much attention to that because the
allocation is short-lived, but matching the surrounding code is
probably better. I've added pfree() in the attached patch.

2. Why do we want to capture the output of getObjectDescription() in a
StringInfo and then add it to the buffer? I think we can pass
getObjectDescription directly as an argument to appendStringInfo()
similar to the code for case AttrDefaultRelationId.

I considered returning strings from helper functions such as
getRelationDescription(), but that would affect a number of similar
helper functions and broaden the scope of the patch.

While working on this, I noticed that the string returned by
getObjectDescription() in the AttrDefaultRelationId case is not
pfree'd. Since that appears unrelated to this patch, I left it as-is.

I also dropped the assertions on the temporary description strings.
They were mainly there while developing the patch and don't seem
necessary in the final version.

Thanks.

I like the idea of pfree'ing the string returned by
getObjectDescriptor() since we also pffree other descriptor strings.
But just doing it for these changes doesn't look consistent.

Attached patchset with some more changes to bring the new code inline
with the surrounding code. The changes are
1. rename objdesc to rel. The latter is used in the surrounding code.
2. getObjectDescriptor() is invoked directly from appendStringInfo.
3. Place InitStringInfo() closer to the appendStringInfo call like
surrounding code

None of these changes are compulsory. If you think any of the changes
are acceptable to you, please provide a single patch absorbing them
into your patch. Otherwise, let the committer decide.

0008 is your patch as is
0009 is my cosmetic changes

The numbering starts from 0008 since I create all SQL/PGQ patches for
various fixes from the same branch. Attached patches are applicable
independently.

--
Best Wishes,
Ashutosh Bapat