On Mon, Mar 10, 2025 at 12:11 PM Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> On Thu, Mar 6, 2025 at 9:27 AM Peter Smith <smithpb2250@gmail.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.
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.
--
Thanks,
Nisha
