Hi Shubham,
Here are some v8 review comments.
======
Commit message
1.
Previously, pg_createsubscriber would fail if any specified publication already
existed. Now, existing publications are reused as-is with their current
configuration, and non-existing publications are createdcautomatically with
FOR ALL TABLES.
~
typo: "createdcautomatically"
======
doc/src/sgml/ref/pg_createsubscriber.sgml
2.
+ <para>
+ Use <option>--dry-run</option> to see which publications will be reused
+ and which will be created before running the actual command.
+ When publications are reused, they will not be dropped during cleanup
+ operations, ensuring they remain available for other uses.
+ Only publications created by
+ <application>pg_createsubscriber</application> on the target server will
+ be cleaned up if the operation fails. Publications on the publisher
+ server are never modified or dropped by cleanup operations.
+ </para>
I still find all this confusing, for the following reasons:
* The "dry-run" advice is OK, but the "cleanup of existing pubs" seems
like a totally different topic which has nothing much to do with the
--publication option. I'm wondering if you need to talk about cleaning
existing pubs, maybe that info belongs in the "Notes" part of this
documentation.
* I don't understand how does saying "existing pubs will not be
dropped" reconcile with the --clean=publication option which says it
will drop all publications? They seem contradictory.
======
src/bin/pg_basebackup/pg_createsubscriber.c
check_and_drop_publications:
3.
/*
- * In dry-run mode, we don't create publications, but we still try to drop
- * those to provide necessary information to the user.
+ * Only drop publications that were created by pg_createsubscriber during
+ * this operation. Pre-existing publications are preserved.
*/
- if (!drop_all_pubs || dry_run)
+ if (dbinfo->made_publication)
drop_publication(conn, dbinfo->pubname, dbinfo->dbname,
&dbinfo->made_publication);
3a.
Sorry, but I still have the same question that I had in my previous v7
review. This function logic will already remove all pre-existing
publications when the 'drop_all_pubs' variable is true. It seems
contrary to the commit message that says "When publications are
reused, they are never dropped during cleanup operations, ensuring
pre-existing publications remain available for other uses."
~
3b.
Kind of similar to the previous review -- If the 'drop_all_pubs'
variable is true, then AFAICT this code attempts to drop again one of
the publications that was already dropped in the earlier part of this
function?
~
3c.
If this drop logic is broken wrt the intended cleanup rules then it
also means more/better --clean option tests are needed, otherwise how
was this passing the tests?
======
Kind Regards,
Peter Smith
Fujitsu Australia
