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