From: Mats Kindahl <mats.kindahl@gmail.com>
To: David Rowley <dgrowleyml@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Use stack-allocated StringInfoData
Date: Thu, 6 Nov 2025 08:37:58 +0100
Message-ID: <71b9d87a-aa3f-4d5e-bc40-e9e4e60e1c82@gmail.com> (raw)
In-Reply-To: <CAApHDvrZnM28wa2VY58cvtY0y9XbMhKJH4m=ga3c1wfsx=MF4Q@mail.gmail.com>
References: <4379aac8-26f1-42f2-a356-ff0e886228d3@gmail.com>
<CAApHDvr2URad5FRknDECPsmt-FT3LukvXDbv6MzgW+_L842+xA@mail.gmail.com>
<1865ee50-8290-4062-b062-5f5ab17364f2@gmail.com>
<CAApHDvpTAnwFWxJ3NR3Xx6gqY_iaV+YsCY7=41oJJ3h-urGESQ@mail.gmail.com>
<04a6a8b3-7572-4021-aaf8-5d3643bc9dac@gmail.com>
<CAApHDvrZnM28wa2VY58cvtY0y9XbMhKJH4m=ga3c1wfsx=MF4Q@mail.gmail.com>
On 11/5/25 12:01, David Rowley wrote:
> On Tue, 4 Nov 2025 at 21:25, Mats Kindahl<mats.kindahl@gmail.com> wrote:
>> Here is an updated patch that I checked manually for unnecessary
>> whitespace changes.
> Thanks for updating the patch.
>
>> 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.
> Yeah, I think you must mean in build_backup_content(). I noticed that too.
Yes, that is the one I meant.
>> 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.
> It's not your fault, but check_publications_origin_tables() looks like
> a mess. It seems like excessive use of additional code just to avoid
> two separate ereport() calls. Might be worth a follow-up patch to
> clean that up.
Yeah, I have noted that too, but I wanted to avoid making reviews more
complicated than necessary. I thought it might be better to do as a
separate effort.
I'll take a look at it.
> The patch looks good to me. The only thing I'd adjust is to get rid of
> the "data" variable from build_backup_content(), which I think you
> mentioned above. I can take care of that one.
Thanks David.
Best wishes,
Mats Kindahl
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: mats.kindahl@gmail.com, dgrowleyml@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Use stack-allocated StringInfoData
In-Reply-To: <71b9d87a-aa3f-4d5e-bc40-e9e4e60e1c82@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox