StringInfo fixes, v19 edition. Plus a few oddities

David Rowley <dgrowleyml@gmail.com> writes:

append_tuple_value_detail() contains some pretty weird translation
strings. The code currently does:

/* translator: This is the terminator of a conflict message */
appendStringInfoString(buf, _("."));

I'm not a translator, but isn't that going to be pretty hard to know
what to do with, given that the same string could be used for anything
else in the code and need something completely different done? That
same function also contains a _(": ") and _(", ") which seem equally
hard to deal with.

Yeah, this seems to me to fly in the face of our style guide's
rule about "don't construct messages out of parts". And the
reason for the rule is precisely that stuff like this poses
insoluble problems for translators.

I think it'd be reasonable to make this helper function build
a string like
(value, value, value)
with the punctuation being untranslated on the grounds that this
is standard SQL notation. Then that could be plugged into a
translatable message that might hopefully represent a full sentence
with a %s for the tuple data.

regards, tom lane

Hi,

On 2026-04-12 12:40:53 +1200, David Rowley wrote:

0001 is the code changes I used to find all these anomalies. It's not
for commit, but attached for reference, or for anyone else who wants
to play around with it.

I wonder if we should make at least "appending just the format string without
arguments" one permanently impossible for appendStringInfo(), instead of
playing whack-a-mole every release. Your wrapper macro

extern void appendStringInfoInternal(StringInfo str, const char *fmt, ...) pg_attribute_printf(2, 3);
#define appendStringInfo(str, fmt, ...) \
appendStringInfoInternal(str, fmt, __VA_ARGS__)

would give you compiler errors if you call appendStringInfo() without
an argument after fmt. It's not the greatest error message, but it'd probably
not confuse people too much?

../../../../../home/andres/src/postgresql/contrib/pg_plan_advice/pgpa_trove.c: In function 'pgpa_trove_append_flags':
../../../../../home/andres/src/postgresql/src/include/lib/stringinfo.h:202:55: error: expected expression before ')' token
202 | appendStringInfoInternal(str, fmt, __VA_ARGS__)
| ^
../../../../../home/andres/src/postgresql/contrib/pg_plan_advice/pgpa_trove.c:349:17: note: in expansion of macro 'appendStringInfo'
349 | appendStringInfo(buf, "matched");
| ^~~~~~~~~~~~~~~~

I guess the export in libpq makes it a bit harder to do this for pqexp.

I don't see any reason not to push 0003, but will happily take
comments in the meantime. I'm not planning on pushing 0002, but things
there might be worth discussing. I'm including Amit for the
append_tuple_value_detail() translation string part from 48efefa6ca.

Any reason you didn't have the relevant outfuncs.c fixes from 0001 - which
iiuc is not intended to be committed - in 0003? Pointlessly using
appendStringInfo() where appendStringInfoString() would have done there is
probably the by far most impactful misuse, performance wise.

Specifically WRITE_CHAR_FIELD(), WRITE_FLOAT_FIELD() seem like they should use
appendStringInfoString()? We have a fair amount of both in node trees.

Greetings,

Andres Freund

On Mon, 13 Apr 2026 at 02:51, Andres Freund <andres@anarazel.de> wrote:

I wonder if we should make at least "appending just the format string without
arguments" one permanently impossible for appendStringInfo(), instead of
playing whack-a-mole every release. Your wrapper macro

I'm open to considering having 0001 or portions of it in core. My
patch isn't ideal, though. For example, if you look at what I had to
do in xml.c. Maybe that wouldn't be quite as bad if I'd come up with a
better name than appendStringInfoInternal(). There's also all the
other cruddy changes in there, like WRITE_*_FIELD() stuff, which I had
to make call the internal function to get it to compile.

extern void appendStringInfoInternal(StringInfo str, const char *fmt, ...) pg_attribute_printf(2, 3);
#define appendStringInfo(str, fmt, ...) \
appendStringInfoInternal(str, fmt, __VA_ARGS__)

would give you compiler errors if you call appendStringInfo() without
an argument after fmt. It's not the greatest error message, but it'd probably
not confuse people too much?

If we went and committed that macro, why wouldn't we include the
StaticAssert part too? That'd also check for "%s". I suppose some
compilers don't have __builtin_constant_p(), so I suppose it might be
annoying if you test with one of those compilers and then only get a
failure after committing.

Any reason you didn't have the relevant outfuncs.c fixes from 0001 - which
iiuc is not intended to be committed - in 0003? Pointlessly using
appendStringInfo() where appendStringInfoString() would have done there is
probably the by far most impactful misuse, performance wise.

Specifically WRITE_CHAR_FIELD(), WRITE_FLOAT_FIELD() seem like they should use
appendStringInfoString()? We have a fair amount of both in node trees.

I did miss those. I wrote the 0001 patch a long time ago and I don't
remember noticing that those could be improved. I'm keen to fix those,
but I'm just not sure that's valid post-freeze work since it's not
touching new-to-v19 code. All the other stuff I did in 0003 is
justified via it being all new code.

David

On Sun, 12 Apr 2026 at 12:40, David Rowley <dgrowleyml@gmail.com> wrote:

All of these changes in 0003 are in code that was touched since the
last time I fixed these in 928394b66. However, the test_escape.c one
added in 627acc3ca is in v18 as it was added after 928394b66 and
before we branched for v19. Almost all of the describe.c ones were
introduced in 41d69e6dc. These ones technically did exist in prior
versions. They're only being highlighted now because the function
changed from printfPQExpBuffer to appendPQExpBuffer, and I must not
have made my StaticAsserts work for printfPQExpBuffer. I left these
in anyway since that code was touched in v19, and future bug fixes
might conflict because of those changes anyway.

I've pushed the 0003 patch.

David

On Sun, 12 Apr 2026 at 06:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Rowley <dgrowleyml@gmail.com> writes:

append_tuple_value_detail() contains some pretty weird translation
strings. The code currently does:

/* translator: This is the terminator of a conflict message */
appendStringInfoString(buf, _("."));

I'm not a translator, but isn't that going to be pretty hard to know
what to do with, given that the same string could be used for anything
else in the code and need something completely different done? That
same function also contains a _(": ") and _(", ") which seem equally
hard to deal with.

Yeah, this seems to me to fly in the face of our style guide's
rule about "don't construct messages out of parts". And the
reason for the rule is precisely that stuff like this poses
insoluble problems for translators.

I think it'd be reasonable to make this helper function build
a string like
(value, value, value)
with the punctuation being untranslated on the grounds that this
is standard SQL notation. Then that could be plugged into a
translatable message that might hopefully represent a full sentence
with a %s for the tuple data.

I've updated the code to avoid constructing messages from translated
fragments. append_tuple_value_detail() now only builds a SQL-style
tuple string like (v1, v2, v3) without any translation or punctuation.
All punctuation and sentence construction are moved to the callers,
which now use a single translatable string with a %s placeholder for
the tuple data.
Attached patch has the changes for the same.

Regards,
Vignesh

On Thursday, April 23, 2026 12:03 AM vignesh C <vignesh21@gmail.com> wrote:

On Sun, 12 Apr 2026 at 06:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Rowley <dgrowleyml@gmail.com> writes:

append_tuple_value_detail() contains some pretty weird translation
strings. The code currently does:

/* translator: This is the terminator of a conflict message */
appendStringInfoString(buf, _("."));

I'm not a translator, but isn't that going to be pretty hard to know
what to do with, given that the same string could be used for
anything else in the code and need something completely different
done? That same function also contains a _(": ") and _(", ") which
seem equally hard to deal with.

Yeah, this seems to me to fly in the face of our style guide's rule
about "don't construct messages out of parts". And the reason for the
rule is precisely that stuff like this poses insoluble problems for
translators.

I think it'd be reasonable to make this helper function build a string
like
(value, value, value)
with the punctuation being untranslated on the grounds that this is
standard SQL notation. Then that could be plugged into a translatable
message that might hopefully represent a full sentence with a %s for
the tuple data.

I've updated the code to avoid constructing messages from translated
fragments. append_tuple_value_detail() now only builds a SQL-style tuple
string like (v1, v2, v3) without any translation or punctuation.
All punctuation and sentence construction are moved to the callers, which
now use a single translatable string with a %s placeholder for the tuple data.
Attached patch has the changes for the same.

The patch seems to change the message content by adding parentheses around the
tuple detail section[1]before: ... row to be deleted: replica identity (a)=(10) After : ... row to be deleted: (replica identity (a)=(10)). However, IIUC, we only need parentheses around the value,
not around the keyword.

[1]: before: ... row to be deleted: replica identity (a)=(10) After : ... row to be deleted: (replica identity (a)=(10))
before: ... row to be deleted: replica identity (a)=(10)
After : ... row to be deleted: (replica identity (a)=(10))

Best Regards,
Hou zj

On Thu, 23 Apr 2026 at 16:07, Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Thursday, April 23, 2026 12:03 AM vignesh C <vignesh21@gmail.com> wrote:

On Sun, 12 Apr 2026 at 06:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Rowley <dgrowleyml@gmail.com> writes:

append_tuple_value_detail() contains some pretty weird translation
strings. The code currently does:

/* translator: This is the terminator of a conflict message */
appendStringInfoString(buf, _("."));

I'm not a translator, but isn't that going to be pretty hard to know
what to do with, given that the same string could be used for
anything else in the code and need something completely different
done? That same function also contains a _(": ") and _(", ") which
seem equally hard to deal with.

Yeah, this seems to me to fly in the face of our style guide's rule
about "don't construct messages out of parts". And the reason for the
rule is precisely that stuff like this poses insoluble problems for
translators.

I think it'd be reasonable to make this helper function build a string
like
(value, value, value)
with the punctuation being untranslated on the grounds that this is
standard SQL notation. Then that could be plugged into a translatable
message that might hopefully represent a full sentence with a %s for
the tuple data.

I've updated the code to avoid constructing messages from translated
fragments. append_tuple_value_detail() now only builds a SQL-style tuple
string like (v1, v2, v3) without any translation or punctuation.
All punctuation and sentence construction are moved to the callers, which
now use a single translatable string with a %s placeholder for the tuple data.
Attached patch has the changes for the same.

The patch seems to change the message content by adding parentheses around the
tuple detail section[1]. However, IIUC, we only need parentheses around the value,
not around the keyword.

[1]
before: ... row to be deleted: replica identity (a)=(10)
After : ... row to be deleted: (replica identity (a)=(10))

I checked this path and confirmed that get_tuple_desc() already wraps
the values in parentheses, so adding another layer here would be
redundant.
Additionally, I have introduced a fallback message "tuple data not
available (insufficient privileges)" to handle cases where the tuple
is not available due to insufficient privileges.

The attached v2 version patch has the changes for the same.

Regards,
Vignesh

On Thu, Apr 23, 2026 at 7:26 PM vignesh C <vignesh21@gmail.com> wrote:

Additionally, I have introduced a fallback message "tuple data not
available (insufficient privileges)" to handle cases where the tuple
is not available due to insufficient privileges.

I think we should avoid adding new fallback message as part of this
patch. We can have a separate discussion to enhance the current
messages, if you feel that will improve user experience.

--
With Regards,
Amit Kapila.

Hi Vignesh.

Some review comments for patch v2-0001.

======

append_tuple_value_detail:

1.
+ if (first)
+ appendStringInfoString(buf, "tuple data not available (insufficient
privileges)");

The logic to use first==true to mean "insufficient privilege" seems
strange. Presumably, the only way to get this message is when the
`continue` of the prior loop happened at *every* iteration.

Isn't it ambiguous? e.g. What if there are multiple tuple_values but
only 1 tuple_value was NULL? Then the boolean `first` will not be
true, so there was something unavailable due to insufficient
privileges that has gone unreported (???).

~~~

errdetail_apply_conflict:

2.
case CT_UPDATE_DELETED:
- appendStringInfoString(&err_detail, _("Could not find the row to be
updated"));

- append_tuple_value_detail(&err_detail,
-   list_make2(remote_desc, search_desc),
-   true);
+ append_tuple_value_detail(&tuple_buf,
+   list_make2(remote_desc, search_desc));
+ appendStringInfo(&err_detail, _("Could not find the row to be
updated: %s.\n"),
+ tuple_buf.data);

Results in a whitespace line after the CT_UPDATE_DELETED: that was not
there before.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, 24 Apr 2026 at 12:30, Peter Smith <smithpb2250@gmail.com> wrote:

Hi Vignesh.

Some review comments for patch v2-0001.

======

append_tuple_value_detail:

1.
+ if (first)
+ appendStringInfoString(buf, "tuple data not available (insufficient
privileges)");

The logic to use first==true to mean "insufficient privilege" seems
strange. Presumably, the only way to get this message is when the
`continue` of the prior loop happened at *every* iteration.

Isn't it ambiguous? e.g. What if there are multiple tuple_values but
only 1 tuple_value was NULL? Then the boolean `first` will not be
true, so there was something unavailable due to insufficient
privileges that has gone unreported (???).

I have removed this from this patch now. We can handle this separately
as it is not required for the current issue.

~~~

errdetail_apply_conflict:

2.
case CT_UPDATE_DELETED:
- appendStringInfoString(&err_detail, _("Could not find the row to be
updated"));

- append_tuple_value_detail(&err_detail,
-   list_make2(remote_desc, search_desc),
-   true);
+ append_tuple_value_detail(&tuple_buf,
+   list_make2(remote_desc, search_desc));
+ appendStringInfo(&err_detail, _("Could not find the row to be
updated: %s.\n"),
+ tuple_buf.data);

Results in a whitespace line after the CT_UPDATE_DELETED: that was not
there before.

Modified

The attached v3 version patch has the changes for the same. This patch
has also addressed Amit's comments from [1]/messages/by-id/CAA4eK1+_Mr1NEzEnGdyZ86r4Bp9a4DwGhnmuKtxwDFv9n5FATg@mail.gmail.com.

[1]: /messages/by-id/CAA4eK1+_Mr1NEzEnGdyZ86r4Bp9a4DwGhnmuKtxwDFv9n5FATg@mail.gmail.com

Regards,
Vignesh

Dear hackers,

Thanks for updating the patch. I have a concern for the handling of colon (:).

In your patch a colon is used as the separator between data and sentence, and
it's not included in the translatable message. The blank exists only after the
colon.

It seems to break syntax rules in other languages. E.g., the blank seems to be
needed on both before and after the character in French [1]``` #: ../../../src/common/logging.c:276 #, c-format msgid "error: " msgstr "erreur : " ```. In other language
the colon may not be used in the first place.

Based on that, How about including a colon in the translatable message?
We may need additional if-else statement but it's a collateral damage.

[1]: ``` #: ../../../src/common/logging.c:276 #, c-format msgid "error: " msgstr "erreur : " ```
```
#: ../../../src/common/logging.c:276
#, c-format
msgid "error: "
msgstr "erreur : "
```

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Mon, 27 Apr 2026 at 11:20, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Dear hackers,

Thanks for updating the patch. I have a concern for the handling of colon (:).

In your patch a colon is used as the separator between data and sentence, and
it's not included in the translatable message. The blank exists only after the
colon.

It seems to break syntax rules in other languages. E.g., the blank seems to be
needed on both before and after the character in French [1]. In other language
the colon may not be used in the first place.

Based on that, How about including a colon in the translatable message?
We may need additional if-else statement but it's a collateral damage.

Yeah, the better way is to go with if-else, the attached v4 version
patch has the changes for the same.

Regards,
Vignesh

Hi Vignesh.

Feedback for v4-0001.

======
src/backend/replication/logical/conflict.c

I guess the following are not strictly the fault of this patch, but I
thought a few of these messages could be improved and made consistent.
Since you are anyway changing most of these messages, now might be the
best time to do it.

e.g.
Current: "Updating the row that was..."

SUGGESTION#1. "Updating a row that was..."
SUGGESTION#2. "Attempting to update a row that was..."
SUGGESTION#3. "The row to be updated was..."

e.g.
Current: "Deleting the row that was..."

SUGGESTION#1. "Deleting a row that was..."
SUGGESTION#2. "Attempting to delete a row that was..."
SUGGESTION#3. "The row to be deleted was..."

~~

And another below (outside the patch) could be updated to be the same:

e.g.
Current: "The row to be updated was deleted by..."

SUGGESTION#1. "Updating a row that was deleted by..."
SUGGESTION#2. "Attempting to update a row that was deleted by..."
SUGGESTION#3. Leave as-is. "The row to be updated was..."

~~~

All those conflict messages should be consistent-looking.

I prefer suggestion #3 because those are also the same as the existing
code comments of the ConflictType enum.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, 28 Apr 2026 at 11:17, Peter Smith <smithpb2250@gmail.com> wrote:

Hi Vignesh.

Feedback for v4-0001.

======
src/backend/replication/logical/conflict.c

I guess the following are not strictly the fault of this patch, but I
thought a few of these messages could be improved and made consistent.
Since you are anyway changing most of these messages, now might be the
best time to do it.

e.g.
Current: "Updating the row that was..."

SUGGESTION#1. "Updating a row that was..."
SUGGESTION#2. "Attempting to update a row that was..."
SUGGESTION#3. "The row to be updated was..."

e.g.
Current: "Deleting the row that was..."

SUGGESTION#1. "Deleting a row that was..."
SUGGESTION#2. "Attempting to delete a row that was..."
SUGGESTION#3. "The row to be deleted was..."

~~

And another below (outside the patch) could be updated to be the same:

e.g.
Current: "The row to be updated was deleted by..."

SUGGESTION#1. "Updating a row that was deleted by..."
SUGGESTION#2. "Attempting to update a row that was deleted by..."
SUGGESTION#3. Leave as-is. "The row to be updated was..."

~~~

All those conflict messages should be consistent-looking.

I prefer suggestion #3 because those are also the same as the existing
code comments of the ConflictType enum.

Since these suggestions are not directly related to the issue being
addressed here, they should be discussed separately and handled
accordingly.

Regards,
Vignesh