public inbox for [email protected]  

help / color / mirror / Atom feed

From: Hayato Kuroda (Fujitsu) <[email protected]>
To: 'Shubham Khanna' <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Shlok Kyal <[email protected]>
Subject: RE: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
Date: Fri, 7 Feb 2025 02:16:02 +0000
Message-ID: <OSCPR01MB1496679AD9C3C47DB30542ECBF5F12@OSCPR01MB14966.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAHv8Rj+1v6257anqijp90f3cdZvSOhkm_ZRQRVU1C_0OYq056g@mail.gmail.com>
References: <CAHv8RjL4OvoYafofTb_U_JD5HuyoNowBoGpMfnEbhDSENA74Kg@mail.gmail.com>
	<OSCPR01MB1496664FDC38DA40A441F449FF5EE2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
	<CAHv8RjJw2xDyWgagdAL7RBUEEEZtKcVJO_gNjQQHkUj8QrmwuQ@mail.gmail.com>
	<CANhcyEUkQybOe=J5KypWQ80deULBLjjsdyP+DF57+BwSni57Vw@mail.gmail.com>
	<CAHv8Rj+1v6257anqijp90f3cdZvSOhkm_ZRQRVU1C_0OYq056g@mail.gmail.com>

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.gma...
[2]: https://www.postgresql.org/message-id/OSCPR01MB1496664FDC38DA40A441F449FF5EE2%40OSCPR01MB14966.jpnpr...
[3]: https://www.postgresql.org/docs/devel/source-format.html

Best regards,
Hayato Kuroda
FUJITSU LIMITED

view thread (12+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: RE: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
  In-Reply-To: <OSCPR01MB1496679AD9C3C47DB30542ECBF5F12@OSCPR01MB14966.jpnprd01.prod.outlook.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox