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