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

Hi Shubham.

Some review comments for patch v16-0001.

======
doc/src/sgml/ref/pg_createsubscriber.sgml

1.
+     <term><option>-c</option></term>
+     <term><option>--drop-all-publications</option></term>

Is 'c' the best switch choice letter for this option? It doesn't seem
intuitive, but unfortunately, I don't have any better ideas since d/D
and p/P are already being used.

======
src/bin/pg_basebackup/pg_createsubscriber.c

CreateSubscriberOptions:

2.
  SimpleStringList sub_names; /* list of subscription names */
  SimpleStringList replslot_names; /* list of replication slot names */
  int recovery_timeout; /* stop recovery after this time */
+ bool drop_publications; /* drop all publications */

Now that you have modified everywhere to rename the parameter as
'drop_all_publications', I think you should change this field name too
so everything is consistent.

~~~

3.
 static void create_publication(PGconn *conn, struct LogicalRepInfo *dbinfo);
-static void drop_publication(PGconn *conn, struct LogicalRepInfo *dbinfo);
+static void drop_publication_by_name(PGconn *conn, const char *pubname,
+ struct LogicalRepInfo *dbinfo);
+static void check_and_drop_existing_publications(PGconn *conn,
+ struct LogicalRepInfo *dbinfo);

I think the parameters of drop_publication_by_name should be reordered
like "(PGconn *conn, struct LogicalRepInfo *dbinfo, const char
*pubname)", to make the 1st 2 params consistent with other functions.
I wrote this already in my previous review. You replied "Fixed"
[1-#5b], but it isn't.

~~~

setup_subscriber:

4.
 /*
  * 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' parameter is true, existing
+ * publications on the subscriber will be dropped before creating new
+ * subscriptions.
  */

The comment refers to 'drop_publications', but the parameter name is
'drop_all_publications'

~~~

5.
  if (PQresultStatus(res) != PGRES_COMMAND_OK)
  {
  pg_log_error("could not drop publication \"%s\" in database \"%s\": %s",
- dbinfo->pubname, dbinfo->dbname, PQresultErrorMessage(res));
- dbinfo->made_publication = false; /* don't try again. */
+ pubname, dbinfo->dbname, PQresultErrorMessage(res));
+
+ if (strcmp(pubname, dbinfo->pubname) == 0)
+ dbinfo->made_publication = false; /* don't try again. */

5a.
IMO this strcmp code needs a comment to say what this logic is doing.
e.g. IIUC dbinfo->pubname is the name of the internal FOR ALL TABLES
publication that the tool had created.

~

5b.
This logic seems flawed to me. Maybe I am mistaken, but AFAIK when you
say '--drop-all-publication' that is going to drop all the existing
publications but it is *also* going to drop the internally created FOR
ALL TABLES publication. Therefore, to prevent the exit handler from
attempting to double-delete the same internal publication don't you
need to set dbinfo->made_publication = false also in the case of
*successful* drop of dbinfo->pubname?

~

5c. Consider maybe also checking if made_publication is true, to avoid
making unnecessary calls to strcmp e.g.
if (dbinfo->made_publication && strcmp(pubname, dbinfo->pubname) == 0)

======
.../t/040_pg_createsubscriber.pl

6.
The test code remains difficult to review because I can't see the
forest for the trees due to the dozens of S->S1 node name changes.
These name changes are unrelated to the new feature so please separate
them into a different prerequisite patch so we can just focus on what
changes were made just for --drop-all-publications. I know you already
said you are working on it [1-#7], but multiple patch versions have
been posted since you said that.

======
[1] v13 review
https://www.postgresql.org/message-id/CAHv8RjKmf5bAcgTmGToWSn_Fx%2BO2y8qCiLScBXvBTD0D5gX2sw%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia