Horton

Thread

Commits

  1. Improve various new-to-v18 appendStringInfo calls

  2. Fixup various StringInfo function usages

  1. Improve a few appendStringInfo calls new to v18

    David Rowley <dgrowleyml@gmail.com> — 2025-04-10T03:51:15Z

    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. 0002 contains the fixes to commit.

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

David

  2. Re: Improve a few appendStringInfo calls new to v18

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-04-10T08:24:36Z

    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)

  3. Re: Improve a few appendStringInfo calls new to v18

    Nathan Bossart <nathandbossart@gmail.com> — 2025-04-10T14:51:53Z

    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].
In most cases, the benefits are probably negligible, but StringInfo is
sometimes used in hot paths.

[0] https://postgr.es/m/20231218164135.GA530790%40nathanxps13

-- 
nathan

  4. Re: Improve a few appendStringInfo calls new to v18

    David Rowley <dgrowleyml@gmail.com> — 2025-04-10T22:10:34Z

    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

  5. Re: Improve a few appendStringInfo calls new to v18

    David Rowley <dgrowleyml@gmail.com> — 2025-04-10T22:40:57Z

    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] https://postgr.es/m/20231218164135.GA530790%40nathanxps13

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.

David

[1] https://postgr.es/m/flat/a0086cfc-ff0f-2827-20fe-52b591d2666c%40enterprisedb.com

  6. Re: Improve a few appendStringInfo calls new to v18

    Peter Eisentraut <peter@eisentraut.org> — 2025-04-11T07:07:04Z

    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);

?

  7. Re: Improve a few appendStringInfo calls new to v18

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-11T13:45:18Z

    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

  8. Re: Improve a few appendStringInfo calls new to v18

    Nathan Bossart <nathandbossart@gmail.com> — 2025-04-11T14:41:41Z

    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