> On Nov 5, 2025, at 19:01, David Rowley <[email protected]> wrote:
>
> 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.
No objection. But I just find that the patch missed one place in
check_publications():
```
if (list_length(publicationsCopy))
{
/* Prepare the list of non-existent publication(s) for error
message. */
StringInfo pubnames = makeStringInfo();
GetPublicationsStr(publicationsCopy, pubnames, false);
ereport(WARNING,
errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg_plural("publication %s does not exist on
the publisher",
"publications %s do
not exist on the publisher",
list_length(publicationsCopy),
pubnames->data));
}
```
This “pubnames” can be replaced as well, and “pfree(pubnames.data)” should be
added after “ereport”. As one place in the function has been replaced in this
patch, I guess we should not leave this place to a separate patch.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
