RE: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.

From: "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>
To: 'Shubham Khanna' <khannashubham1197(at)gmail(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com>
Subject: RE: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
Date: 2025-02-07 02:16:02
Message-ID: OSCPR01MB1496679AD9C3C47DB30542ECBF5F12@OSCPR01MB14966.jpnprd01.prod.outlook.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Dear Shubham,

Thanks for updating the patch.

Previously you told that you had a plan to extend the patch to drop other replication
objects [1], but I think it is not needed. pg_createsubscriber has already been
able to drop the existing subscrisubscriptions in check_and_drop_existing_subscriptions().
As for the replication slot, I have told in [2], it would be created intentionally
thus I feel it should not be dropped.
Thus I regard the patch does not have concrete extending plan.

Below part contains my review comment.

01. Option name

Based on the above discussion, "--cleanup-publisher-objects" is not suitable because
it won't drop replication slots. How about "--cleanup-publications"?

02. usage()
```
+ printf(_(" -C --cleanup-publisher-objects drop all publications on the logical replica\n"));
```

s/logical replica/subscriber

03. drop_all_publications
```
+/* Drops all existing logical replication publications from all subscriber
+ * databases
+ */
+static void
```

Initial line of the comment must be blank [3].

04. main
```
+ {"cleanup-publisher-objects", no_argument, NULL, 'C'},
```

Is there a reason why upper case is used? I feel lower one is enough.

05. main
```
+ /* Drop publications from the subscriber if requested */
+ if (opt.cleanup_publisher_objects)
+ drop_all_publications(dbinfo);
```

After considering more, I noticed that we have already called drop_publication()
in the setup_subscriber(). Can we call drop_all_publications() there instead when
-C is specified?

06. 040_pg_createsubscriber.pl

```
+$node_s->start;
+# Create publications to test it's removal
+$node_p->safe_psql($db1, "CREATE PUBLICATION test_pub FOR ALL TABLES;");
+$node_p->safe_psql($db1, "CREATE PUBLICATION test_pub2 FOR ALL TABLES;");
+
+# Verify the existing publications
+my $pub_count_before =
+ $node_s->safe_psql($db1, "SELECT COUNT(*) FROM pg_publication;");
+is($pub_count_before, '2',
+ 'two publications created before --cleanup-publisher-objects is run');
+
+$node_s->stop;
```

I feel it requires unnecessary startup and shutdown. IIUC, creating publications and check
counts can be before stopping the node_s, around line 331.

07. 040_pg_createsubscriber.pl

```
+$node_p->safe_psql($db1, "CREATE PUBLICATION test_pub FOR ALL TABLES;");
+$node_p->safe_psql($db1, "CREATE PUBLICATION test_pub2 FOR ALL TABLES;");
+
+# Verify the existing publications
+my $pub_count_before =
+ $node_s->safe_psql($db1, "SELECT COUNT(*) FROM pg_publication;");
+is($pub_count_before, '2',
+ 'two publications created before --cleanup-publisher-objects is run');
+
```

Also, there is a possibility that CREATE PUBLICATION on node_p is not replicated yet
when SELECT COUNT(*) is executed. Please wait for the replay.

[1]: https://www.postgresql.org/message-id/CAHv8RjL4OvoYafofTb_U_JD5HuyoNowBoGpMfnEbhDSENA74Kg%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/OSCPR01MB1496664FDC38DA40A441F449FF5EE2%40OSCPR01MB14966.jpnprd01.prod.outlook.com
[3]: https://www.postgresql.org/docs/devel/source-format.html

Best regards,
Hayato Kuroda
FUJITSU LIMITED

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message jian he 2025-02-07 02:28:24 Re: Document NULL
Previous Message Sami Imseih 2025-02-07 01:52:53 Re: [PATCH] Optionally record Plan IDs to track plan changes for a query