Re: Add support for specifying tables in pg_createsubscriber. - Mailing list pgsql-hackers

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