Improve a few appendStringInfo calls new to v18

On 10/04/2025 06:51, David Rowley wrote:

Looks like v18 has grown a few appendStringInfo misusages, e.g. using
appendStringInfo() when no formatting is needed or just using format
"%s" instead of using appendStringInfoString().

I've attached a couple of patches. The 0001 is just my method for
finding these, not for commit.

Clever!

0002 contains the fixes to commit.

Any objections to doing this soonish? Or in a few weeks?

Sure, let's do it. Why would we wait?

--
Heikki Linnakangas
Neon (https://neon.tech)

On Thu, Apr 10, 2025 at 11:24:36AM +0300, Heikki Linnakangas wrote:

On 10/04/2025 06:51, David Rowley wrote:

Any objections to doing this soonish? Or in a few weeks?

Sure, let's do it. Why would we wait?

+1. You did something similar for v17 (commit 8461424), and it seems like
an entirely reasonable post-feature-freeze routine to me.

This probably isn't v18 material, but this reminds me of my idea to change
appendStringInfoString() into a macro for appendBinaryStringInfo() so that
the compiler can remove the runtime strlen() calls for string literals [0]/messages/by-id/20231218164135.GA530790@nathanxps13.
In most cases, the benefits are probably negligible, but StringInfo is
sometimes used in hot paths.

[0]: /messages/by-id/20231218164135.GA530790@nathanxps13

--
nathan

On Thu, 10 Apr 2025 at 20:24, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On 10/04/2025 06:51, David Rowley wrote:

Any objections to doing this soonish? Or in a few weeks?

Sure, let's do it. Why would we wait?

Great. Pushed. Was considering waiting as I didn't know if there was
a revert-fest looming or not, and this may have conflicted a bit. In
hindsight, likely not a good thing to optimise for.

David

On Fri, 11 Apr 2025 at 02:51, Nathan Bossart <nathandbossart@gmail.com> wrote:

This probably isn't v18 material, but this reminds me of my idea to change
appendStringInfoString() into a macro for appendBinaryStringInfo() so that
the compiler can remove the runtime strlen() calls for string literals [0].
In most cases, the benefits are probably negligible, but StringInfo is
sometimes used in hot paths.

[0] /messages/by-id/20231218164135.GA530790@nathanxps13

That one has come up a few times. The most lengthy discussion I
remember was in [1]/messages/by-id/flat/a0086cfc-ff0f-2827-20fe-52b591d2666c@enterprisedb.com. It didn't come to anything, but I don't think
there were any objections to it, so maybe we should just do it.

In the thread I did some measurements of binary size increases. For
non-compile-time consts, it does mean putting the strlen() call in the
calling function, which is a bit of overhead in terms of size. The
macro trick I suggested should have fixed that, but I admit the macro
is a bit ugly. The macro version also still has the overhead of having
to pass the length of the string when it detects a compile-time const.

David

[1]: /messages/by-id/flat/a0086cfc-ff0f-2827-20fe-52b591d2666c@enterprisedb.com

On 10.04.25 05:51, David Rowley wrote:

Looks like v18 has grown a few appendStringInfo misusages, e.g. using
appendStringInfo() when no formatting is needed or just using format
"%s" instead of using appendStringInfoString().

Would it be useful to augment appendStringInfo() something like this:

if (VA_ARGS_NARGS() == 0)
return appendStringInfoString(str, fmt);

?

Peter Eisentraut <peter@eisentraut.org> writes:

Would it be useful to augment appendStringInfo() something like this:

if (VA_ARGS_NARGS() == 0)
return appendStringInfoString(str, fmt);

That would change the behavior in edge cases, for instance
appendStringInfo(str, "foo%%bar"). Maybe we'd never hit those,
but on the whole I'm not in love with the idea.

regards, tom lane

On Fri, Apr 11, 2025 at 10:40:57AM +1200, David Rowley wrote:

On Fri, 11 Apr 2025 at 02:51, Nathan Bossart <nathandbossart@gmail.com> wrote:

This probably isn't v18 material, but this reminds me of my idea to change
appendStringInfoString() into a macro for appendBinaryStringInfo() so that
the compiler can remove the runtime strlen() calls for string literals [0].
In most cases, the benefits are probably negligible, but StringInfo is
sometimes used in hot paths.

That one has come up a few times. The most lengthy discussion I
remember was in [1]. It didn't come to anything, but I don't think
there were any objections to it, so maybe we should just do it.

In the thread I did some measurements of binary size increases. For
non-compile-time consts, it does mean putting the strlen() call in the
calling function, which is a bit of overhead in terms of size. The
macro trick I suggested should have fixed that, but I admit the macro
is a bit ugly. The macro version also still has the overhead of having
to pass the length of the string when it detects a compile-time const.

Thanks for the additional context.

--
nathan