Обсуждение: Use stack-allocated StringInfoData

Hi all,

While working on other things I noted that we have a lot of cases where a StringInfo instance is allocated dynamically even when it is either thrown away or destroyed at the end, which seems unnecessary, that is, instead of using:

       StringInfo info = makeStringInfo();
       ...
       appendStringInfo(info, ...);
       ...
       return info->data;

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.

[1]: https://www.postgresql.org/message-id/8895cba9-48cf-40fe-9c47-9b43ec6b2ab3%40gmail.com

Best wishes,
Mats Kindahl

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
versionthat might be interesting to include regardless of other changes. The patch is applied and one case that
couldn'tbe matched is manually fixed. 

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. 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). 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?

David

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

On Tue, 4 Nov 2025 at 09:20, Mats Kindahl <mats.kindahl@gmail.com> wrote:
> This is using Coccinelle to transform the text based on a semantic patch (which is included in the patch).
Unfortunatelyit mess with the whitespace a bit so it is necessary to run pg_intent after.  I'm surprised that there are
whitespacechanges that should not be there, but I'll take a look. 

pgindent likely won't put back the types of changes that were made
here. I think you're going to have to manually review anything that
comes out of any automated tool, at least certainly until there's some
confidence that the scripts are configured correctly. Loading extra
work onto reviewers or committers that you could have easily done
yourself isn't really a sustainable thing. We just lack bandwidth for
that, plus it's bad etiquette.

A few tips: It's fine to post rough patches to demonstrate ideas to
the list, but make that clear when doing so. If your intention is that
what you're sending gets committed, then please aim for as high a
quality as you can. There just won't be any long-term patience for
series of unvetted-by-human transformation patches from tools such as
Coccinelle around here, especially so if you expect the community to
vet them for you. I'm not saying that's what you're doing, but if you
are, please take this advice.

David

On 11/3/25 22:32, David Rowley wrote:
> On Tue, 4 Nov 2025 at 09:20, Mats Kindahl <mats.kindahl@gmail.com> wrote:
>> This is using Coccinelle to transform the text based on a semantic patch (which is included in the patch).
Unfortunatelyit mess with the whitespace a bit so it is necessary to run pg_intent after.  I'm surprised that there are
whitespacechanges that should not be there, but I'll take a look.
 
> pgindent likely won't put back the types of changes that were made
> here. I think you're going to have to manually review anything that
> comes out of any automated tool, at least certainly until there's some
> confidence that the scripts are configured correctly. Loading extra
> work onto reviewers or committers that you could have easily done
> yourself isn't really a sustainable thing. We just lack bandwidth for
> that, plus it's bad etiquette.
>
> A few tips: It's fine to post rough patches to demonstrate ideas to
> the list, but make that clear when doing so. If your intention is that
> what you're sending gets committed, then please aim for as high a
> quality as you can. There just won't be any long-term patience for
> series of unvetted-by-human transformation patches from tools such as
> Coccinelle around here, especially so if you expect the community to
> vet them for you. I'm not saying that's what you're doing, but if you
> are, please take this advice.

Hi David,

My apologies for not being clear. I understand the situation and thank 
you for the advice. I'll keep that in mind in the future.

Here is an updated patch that I checked manually for unnecessary 
whitespace changes.

I also checked that "destroyStringInfo(info)" was handled correctly, 
which would be replacing it with a call to "pfree(info.data)" since the 
StringInfoData structure is now on the stack but the data buffer needs 
to be released since this is what a call to destroyStringInfo() would do.

Removing any calls of "pfree(info)" when "info" was changed to 
"StringInfoData" since it is no longer a dynamically allocated variable. 
Note that in this case it is not necessary to free "info.data" since the 
original code does not do that.

There is one such case affected by this patch, and there the buffer 
pointer is copied to another variable and then returned. This can 
probably be improved by just returning the buffer pointer directly 
without intermediate assignment to a variable, but I avoided this to 
make reviewing the code easier. It is easy to add this change either now 
or later and should not affect the generated code except at very low 
optimization levels.

I also added fixes manually inside check_publications_origin_tables, 
check_publications, and fetch_relation_list. In 
check_publication_origin_table three StringInfo was dynamically 
allocated but not released after. In check_publications and 
fetch_relation_list there were extra cases of using a dynamically 
allocated StringInfo that was not necessary.

Best wishes,
Mats Kindahl