Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility. - Mailing list pgsql-hackers

On Mon, Mar 10, 2025 at 4:31 PM Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> > 2) This part of code has another bug:
> >  "dbinfos.dbinfo->made_publication = false;" incorrectly modifies
> > dbinfo[0], even if the failure occurs in other databases (dbinfo[1],
> > etc.). Since cleanup_objects_atexit() checks made_publication per
> > database, this could lead to incorrect behavior.
> > Solution: Pass the full 'dbinfo' structure to
> > drop_publication_by_name() , and use it accordingly.
> >
> > --
>
> Fixed.
>
> The attached patch contains the suggested changes.
>

Thanks for the patch. Here are few comments for the new change -
1)
+static void check_and_drop_existing_publications(PGconn *conn, struct
LogicalRepInfo *dbinfo);
 - move the second parameter to the next line to maintain 80 char length.

2)
  pg_log_error("could not drop publication \"%s\" in database \"%s\": %s",
- dbinfo->pubname, dbinfo->dbname, PQresultErrorMessage(res));
+ pubname, dbinfo->dbname,
+ PQresultErrorMessage(res));

You can keep "PQresultErrorMessage(res));" in the previous line, it
will still be < 80 chars.

3) The new IF check -
+           if (strcmp(pubname, dbinfos.dbinfo->pubname) == 0)
+               dbinfo->made_publication = false;   /* don't try again. */

always compares dbinfo[0]->pubname, but it should compare 'pubname'
with the respective database's publication. Please correct it as -
        if (strcmp(pubname, dbinfo->pubname) == 0)


--
Thanks,
Nisha