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

On Mon, Mar 10, 2025 at 3:09 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> On Mon, Mar 10, 2025 at 12:11 PM Shubham Khanna
> <khannashubham1197(at)gmail(dot)com> wrote:
> >
> > On Thu, Mar 6, 2025 at 9:27 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > >
> > > 6.
> > > - dbinfo->pubname, dbinfo->dbname, PQresultErrorMessage(res));
> > > - dbinfo->made_publication = false; /* don't try again. */
> > > + pubname, dbname, PQresultErrorMessage(res));
> > > + dbinfos.dbinfo->made_publication = false; /* don't try again. */
> > >
> > > Something about this flag assignment seems odd to me. IIUC
> > > 'made_publications' is for removing the cleanup_objects_atexit to be
> > > able to remove the special publication implicitly made by the
> > > pg_createsubscriber. But, AFAIK we can also get to this code via the
> > > --cleanup-existing-publication switch, so actually it might be one of
> > > the "existing" publication DROPS that has failed. If so, then the
> > > "don't try again comment" is misleading because it may not be that
> > > same publication "again" at all.
> > >
> >
> > I agree with your point. The current comment is misleading, as the
> > failure could be related to an existing publication drop via
> > --cleanup-existing-publications now --drop-all-publications, not just
> > the publication created by pg_createsubscriber.
> > This issue is still open, and I will address it in the next version of
> > the patch.
> >
>
> 1) We should set "made_publication = false" only if the failure occurs
> for the special publication implicitly created by pg_createsubscriber.
> If the error is in any other publication, this special publication
> should still be cleaned up by the cleanup_objects_atexit.
>

Fixed.

> 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 and regards,
Shubham Khanna.