Re: StringInfo fixes, v19 edition. Plus a few oddities

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