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
