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
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 |