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
