Content-Type: multipart/alternative;
 boundary="------------v7qSyEjVIb80eGlPRtKtZ8KR"
Message-ID: <1865ee50-8290-4062-b062-5f5ab17364f2@gmail.com>
Date: Mon, 3 Nov 2025 21:20:32 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Use stack-allocated StringInfoData
To: David Rowley <dgrowleyml@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
References: <4379aac8-26f1-42f2-a356-ff0e886228d3@gmail.com>
 <CAApHDvr2URad5FRknDECPsmt-FT3LukvXDbv6MzgW+_L842+xA@mail.gmail.com>
Content-Language: en-US
From: Mats Kindahl <mats.kindahl@gmail.com>
In-Reply-To: 
 <CAApHDvr2URad5FRknDECPsmt-FT3LukvXDbv6MzgW+_L842+xA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/1865ee50-8290-4062-b062-5f5ab17364f2%40gmail.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------v7qSyEjVIb80eGlPRtKtZ8KR
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


On 11/3/25 11:27, David Rowley wrote:
> On Mon, 3 Nov 2025 at 20:27, Mats Kindahl<mats.kindahl@gmail.com> wrote:
>> We can use
>>
>>         StringInfoData info;
>>         initStringInfo(&info);
>>         ...
>>         appendStringInfo(&info, ...);
>>         ...
>>         return info.data;
>>
>> It was corrected in an earlier commit, but that seems to have been removed so we still have a lot of these cases.
>>
>> I created a semantic patch to capture most of these cases, which is present in [1], but this is a slightly modified version that might be interesting to include regardless of other changes. The patch is applied and one case that couldn't be matched is manually fixed.

Hi David, and thanks for the review.

> I think this is a worthwhile conversion. Are you able to create a more
> complete version of this? The draft version does introduce quite a few
> whitespace changes that aren't wanted.

Absolutely, I'll look into it.

> You've also introduced a memory
> leak in check_publications(), fetch_relation_list(), jsonb_send() and
> xml_errorHandler() (NB: destroyStringInfo() doesn't just pfree the
> memory for the struct, it pfree's the data too).

Ah, that was a mistake from my side. Will fix.

> The patch shouldn't
> be leaving any memory around that the current master is careful to
> pfree.
>
> Do you have any semi-automated method to find these? Or is it a case
> of manually reviewing code with a makeStringInfo() call?

This is using Coccinelle to transform the text based on a semantic patch 
(which is included in the patch). Unfortunately it mess with the 
whitespace a bit so it is necessary to run pg_intent after.  I'm 
surprised that there are whitespace changes that should not be there, 
but I'll take a look.

Thanks again, and best wishes,
Mats Kindahl
--------------v7qSyEjVIb80eGlPRtKtZ8KR
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <p><br>
    </p>
    <div class="moz-cite-prefix">On 11/3/25 11:27, David Rowley wrote:<br>
    </div>
    <blockquote type="cite"
cite="mid:CAApHDvr2URad5FRknDECPsmt-FT3LukvXDbv6MzgW+_L842+xA@mail.gmail.com">
      <pre wrap="" class="moz-quote-pre">On Mon, 3 Nov 2025 at 20:27, Mats Kindahl <a class="moz-txt-link-rfc2396E" href="mailto:mats.kindahl@gmail.com">&lt;mats.kindahl@gmail.com&gt;</a> wrote:
</pre>
      <blockquote type="cite">
        <pre wrap="" class="moz-quote-pre">We can use

       StringInfoData info;
       initStringInfo(&amp;info);
       ...
       appendStringInfo(&amp;info, ...);
       ...
       return info.data;

It was corrected in an earlier commit, but that seems to have been removed so we still have a lot of these cases.

I created a semantic patch to capture most of these cases, which is present in [1], but this is a slightly modified version that might be interesting to include regardless of other changes. The patch is applied and one case that couldn't be matched is manually fixed.
</pre>
      </blockquote>
    </blockquote>
    <p>Hi David, and thanks for the review.</p>
    <p><span style="white-space: pre-wrap">
</span></p>
    <blockquote type="cite"
cite="mid:CAApHDvr2URad5FRknDECPsmt-FT3LukvXDbv6MzgW+_L842+xA@mail.gmail.com">
      <pre wrap="" class="moz-quote-pre">I think this is a worthwhile conversion. Are you able to create a more
complete version of this? The draft version does introduce quite a few
whitespace changes that aren't wanted.</pre>
    </blockquote>
    <p>Absolutely, I'll look into it.</p>
    <blockquote type="cite"
cite="mid:CAApHDvr2URad5FRknDECPsmt-FT3LukvXDbv6MzgW+_L842+xA@mail.gmail.com">
      <pre wrap="" class="moz-quote-pre">You've also introduced a memory
leak in check_publications(), fetch_relation_list(), jsonb_send() and
xml_errorHandler() (NB: destroyStringInfo() doesn't just pfree the
memory for the struct, it pfree's the data too).</pre>
    </blockquote>
    <p>Ah, that was a mistake from my side. Will fix.</p>
    <blockquote type="cite"
cite="mid:CAApHDvr2URad5FRknDECPsmt-FT3LukvXDbv6MzgW+_L842+xA@mail.gmail.com">
      <pre wrap="" class="moz-quote-pre">The patch shouldn't
be leaving any memory around that the current master is careful to
pfree.

Do you have any semi-automated method to find these? Or is it a case
of manually reviewing code with a makeStringInfo() call?</pre>
    </blockquote>
    <p>This is using Coccinelle to transform the text based on a
      semantic patch (which is included in the patch). Unfortunately it
      mess with the whitespace a bit so it is necessary to run pg_intent
      after.  I'm surprised that there are whitespace changes that
      should not be there, but I'll take a look.</p>
    Thanks again, and best wishes,<br>
    Mats Kindahl
  </body>
</html>

--------------v7qSyEjVIb80eGlPRtKtZ8KR--