Re: Use stack-allocated StringInfoData

On 11/5/25 12:01, David Rowley wrote:
> On Tue, 4 Nov 2025 at 21:25, Mats Kindahl<mats(dot)kindahl(at)gmail(dot)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