Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility. - Mailing list pgsql-hackers

On Tue, Mar 4, 2025 at 10:30 AM Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> Fixed the suggested changes. The attached patch contains the required changes.
>

Hi Shubham,

Thanks for the patch! I tested its functionality and didn't find any
issues so far. I'll continue with further testing.
Here are some review comments for v12 patch:

src/bin/pg_basebackup/pg_createsubscriber.c

1)
+ printf(_("  -c  --cleanup-existing-publications\n"
+ "                                  drop all publications on the
subscriber\n"));

Similar to docs, here too, we should clarify that publications will be
dropped only from the specified databases, not all databases.
Suggestion -
"drop all publications from specified databases on the subscriber\n"

2)
@@ -1171,10 +1179,12 @@ check_and_drop_existing_subscriptions(PGconn *conn,
 /*
  * Create the subscriptions, adjust the initial location for logical
  * replication and enable the subscriptions. That's the last step for logical
- * replication setup.
+ * replication setup. If 'drop_publications' is true, existing publications on
+ * the subscriber will be dropped before creating new subscriptions.
  */
 static void
-setup_subscriber(struct LogicalRepInfo *dbinfo, const char *consistent_lsn)
+setup_subscriber(struct LogicalRepInfo *dbinfo, const char *consistent_lsn,
+ bool cleanup_existing_publications)
 {

It is not clear that the 'drop_publications' value comes from the
command. How about replacing it with:
/If 'drop_publications' is/If 'drop_publications' option is/

OR

If you meant to refer to the specific parameter
'cleanup_existing_publications', please use the exact name.

3)
@@ -1195,8 +1205,13 @@ setup_subscriber(struct LogicalRepInfo *dbinfo,
const char *consistent_lsn)
  * Since the publication was created before the consistent LSN, it is
  * available on the subscriber when the physical replica is promoted.
  * Remove publications from the subscriber because it has no use.
+ * Additionally, drop publications existed before this command if
+ * requested.
  */
- drop_publication(conn, &dbinfo[i]);
+ if (cleanup_existing_publications)
+ check_and_drop_existing_publications(conn, dbinfo[i].dbname);
+ else
+ drop_publication_by_name(conn, dbinfo[i].pubname, dbinfo[i].dbname);

The existing comment only refers to removing the new publication
created for the current process and does not mention existing ones.
With this patch changes, it is unclear which publications are being
referenced when saying "Remove publications ..."(plural), and the
phrase "before this command" is ambiguous—it's not clear which command
is being referred to. The comment should be reworded for better
clarity.

Suggestion -

"Since the publication was created before the consistent LSN, it
 remains on the subscriber even after the physical replica is
 promoted. Remove this publication from the subscriber because
 it has no use. Additionally, if requested, drop all pre-existing
 publications."

4)
+/* Drop a single publication, given in dbinfo. */
+static void
+drop_publication_by_name(PGconn *conn, const char *pubname, const char *dbname)
+{

Since "dbinfo" is no longer passed to this function, the function
comments should be updated accordingly. Also, use the same format as
other function comments.
Suggestion-
/*
 * Drop the specified publication of the given database.
 */

5)
+ pg_log_info("dropping publication %s in database \"%s\"", pubname, dbname);
+

Publication names should be enclosed in double quotes ("") in logs, as
previously done.

6)
+ pg_log_error("could not drop publication %s in database \"%s\": %s",
+ pubname, dbname, PQresultErrorMessage(res));

Same as #5, use double quotes for the publication name.

--
Thanks,
Nisha