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.
> 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.
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.
I can push this tomorrow morning UTC+13. I'll leave it til then in
case anyone else has any comments.
David
