Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.

On Mon, Mar 10, 2025 at 5:02 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> On Mon, Mar 10, 2025 at 4:31 PM Shubham Khanna
> <khannashubham1197(at)gmail(dot)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.
>

Fixed.

> 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.
>

Fixed.

> 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)
>
>
> --

Fixed.

The attached patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.