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

Dear Shubham,

I propose adding the --clean-publisher-objects option to the
pg_createsubscriber utility. As discussed in [1], this feature ensures
a clean and streamlined setup of logical replication by removing stale
or unnecessary publications from the subscriber node. These
publications, replicated during streaming replication, become
redundant after converting to logical replication and serve no further
purpose. This patch introduces the drop_all_publications() function,
which efficiently fetches and drops all publications on the subscriber
node within a single transaction.

I think replication slot are also type of 'publisher-objects', but they are not
removed for now: API-name may not be accurate. And...

Additionally, other related objects, such as subscriptions and
replication slots, may also require cleanup. I plan to analyze this
further and include them in subsequent patches.

I'm not sure replication slots should be cleaned up. Apart from other items like
publication/subscription, replication slots are not replicated when it is created
on the primary instance. This means they are intentionally created by DBAs and there
may not be no strong reasons to drop them after the conversion.

Another question is the style of APIs. Do you plan to provide APIs like
'cleanup-subscriber-objects' and 'cleanup-publisher-objects', or just one
'cleanup-logical-replication-objects'?

Regarding the patch:

1.
```
+       The <application>pg_createsubscriber</application> now supports the
+       <option>--clean-publisher-objects</option> to remove all publications on
+       the subscriber node before creating a new subscription.
```

This description is not suitable for the documentation. Something like:

Remove all publications on the subscriber node.

2.
```
+	/* Drop publications from the subscriber if requested */
+	drop_all_publications(dbinfo);
```

This should be called when `opts.clean_publisher_objects` is true.

3.
You said publications are dropped within a single transaction, but the
patch does not do. Which is correct?

4.
```
+# Set up node A as primary
+my $node_a = PostgreSQL::Test::Cluster->new('node_a');
+my $aconnstr = $node_a->connstr;
+$node_a->init(allows_streaming => 'logical');
+$node_a->append_conf('postgresql.conf', 'autovacuum = off');
+$node_a->start;
+
+# Set up node B as standby linking to node A
+$node_a->backup('backup_3');
+my $node_b = PostgreSQL::Test::Cluster->new('node_b');
+$node_b->init_from_backup($node_a, 'backup_3', has_streaming => 1);
+$node_b->append_conf(
+	'postgresql.conf', qq[
+       primary_conninfo = '$aconnstr'
+       hot_standby_feedback = on
+       max_logical_replication_workers = 5
+       ]);
+$node_b->set_standby_mode();
+$node_b->start;
```

I don't think new nodes are needed in the test. Can you reuse node_p/node_s for the purpose?

----------
Best regards,
Haato Kuroda

On Wed, Jan 29, 2025 at 10:42 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Dear Shubham,

I propose adding the --clean-publisher-objects option to the
pg_createsubscriber utility. As discussed in [1], this feature ensures
a clean and streamlined setup of logical replication by removing stale
or unnecessary publications from the subscriber node. These
publications, replicated during streaming replication, become
redundant after converting to logical replication and serve no further
purpose. This patch introduces the drop_all_publications() function,
which efficiently fetches and drops all publications on the subscriber
node within a single transaction.

I think replication slot are also type of 'publisher-objects', but they are not
removed for now: API-name may not be accurate. And...

Additionally, other related objects, such as subscriptions and
replication slots, may also require cleanup. I plan to analyze this
further and include them in subsequent patches.

I'm not sure replication slots should be cleaned up. Apart from other items like
publication/subscription, replication slots are not replicated when it is created
on the primary instance. This means they are intentionally created by DBAs and there
may not be no strong reasons to drop them after the conversion.

Another question is the style of APIs. Do you plan to provide APIs like
'cleanup-subscriber-objects' and 'cleanup-publisher-objects', or just one
'cleanup-logical-replication-objects'?

Thanks for the suggestions, I will keep them in mind while preparing
the 0002 patch for the same.
Currently, I have changed the API to '--cleanup-publisher-objects'.

Regarding the patch:

1.
```
+       The <application>pg_createsubscriber</application> now supports the
+       <option>--clean-publisher-objects</option> to remove all publications on
+       the subscriber node before creating a new subscription.
```

This description is not suitable for the documentation. Something like:

Remove all publications on the subscriber node.

2.
```
+       /* Drop publications from the subscriber if requested */
+       drop_all_publications(dbinfo);
```

This should be called when `opts.clean_publisher_objects` is true.

3.
You said publications are dropped within a single transaction, but the
patch does not do. Which is correct?

4.
```
+# Set up node A as primary
+my $node_a = PostgreSQL::Test::Cluster->new('node_a');
+my $aconnstr = $node_a->connstr;
+$node_a->init(allows_streaming => 'logical');
+$node_a->append_conf('postgresql.conf', 'autovacuum = off');
+$node_a->start;
+
+# Set up node B as standby linking to node A
+$node_a->backup('backup_3');
+my $node_b = PostgreSQL::Test::Cluster->new('node_b');
+$node_b->init_from_backup($node_a, 'backup_3', has_streaming => 1);
+$node_b->append_conf(
+       'postgresql.conf', qq[
+       primary_conninfo = '$aconnstr'
+       hot_standby_feedback = on
+       max_logical_replication_workers = 5
+       ]);
+$node_b->set_standby_mode();
+$node_b->start;
```

Fixed the given comments. The attached patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.

On Wed, 29 Jan 2025 at 15:14, Shubham Khanna
<khannashubham1197@gmail.com> wrote:

On Wed, Jan 29, 2025 at 10:42 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Dear Shubham,

I propose adding the --clean-publisher-objects option to the
pg_createsubscriber utility. As discussed in [1], this feature ensures
a clean and streamlined setup of logical replication by removing stale
or unnecessary publications from the subscriber node. These
publications, replicated during streaming replication, become
redundant after converting to logical replication and serve no further
purpose. This patch introduces the drop_all_publications() function,
which efficiently fetches and drops all publications on the subscriber
node within a single transaction.

I think replication slot are also type of 'publisher-objects', but they are not
removed for now: API-name may not be accurate. And...

Additionally, other related objects, such as subscriptions and
replication slots, may also require cleanup. I plan to analyze this
further and include them in subsequent patches.

I'm not sure replication slots should be cleaned up. Apart from other items like
publication/subscription, replication slots are not replicated when it is created
on the primary instance. This means they are intentionally created by DBAs and there
may not be no strong reasons to drop them after the conversion.

Another question is the style of APIs. Do you plan to provide APIs like
'cleanup-subscriber-objects' and 'cleanup-publisher-objects', or just one
'cleanup-logical-replication-objects'?

Thanks for the suggestions, I will keep them in mind while preparing
the 0002 patch for the same.
Currently, I have changed the API to '--cleanup-publisher-objects'.

Regarding the patch:

1.
```
+       The <application>pg_createsubscriber</application> now supports the
+       <option>--clean-publisher-objects</option> to remove all publications on
+       the subscriber node before creating a new subscription.
```

This description is not suitable for the documentation. Something like:

Remove all publications on the subscriber node.

2.
```
+       /* Drop publications from the subscriber if requested */
+       drop_all_publications(dbinfo);
```

This should be called when `opts.clean_publisher_objects` is true.

3.
You said publications are dropped within a single transaction, but the
patch does not do. Which is correct?

4.
```
+# Set up node A as primary
+my $node_a = PostgreSQL::Test::Cluster->new('node_a');
+my $aconnstr = $node_a->connstr;
+$node_a->init(allows_streaming => 'logical');
+$node_a->append_conf('postgresql.conf', 'autovacuum = off');
+$node_a->start;
+
+# Set up node B as standby linking to node A
+$node_a->backup('backup_3');
+my $node_b = PostgreSQL::Test::Cluster->new('node_b');
+$node_b->init_from_backup($node_a, 'backup_3', has_streaming => 1);
+$node_b->append_conf(
+       'postgresql.conf', qq[
+       primary_conninfo = '$aconnstr'
+       hot_standby_feedback = on
+       max_logical_replication_workers = 5
+       ]);
+$node_b->set_standby_mode();
+$node_b->start;
```

Fixed the given comments. The attached patch contains the suggested changes.

Hi Shubham,

I have reviewed the v2 patch. Here are my comments:

1. Initial of the comment should in smallcase to make it similar to
other comments:
+ bool cleanup_publisher_objects; /* Drop all publications */

2. We should provide a comment for the function.
+static void
+drop_all_publications(const struct LogicalRepInfo *dbinfo)
+{

3. We should declare it as "const char*"
+   char       *search_query = "SELECT pubname FROM pg_catalog.pg_publication;";
+

4. Instead of warning we should throw an error here:
+       if (PQresultStatus(res) != PGRES_TUPLES_OK)
+       {
+           pg_log_warning("could not obtain publication information: %s",
+                          PQresultErrorMessage(res));
+

5. Should we throw an error in this case as well?
+               if (PQresultStatus(res_for_drop) != PGRES_COMMAND_OK)
+               {
+                   pg_log_warning("could not drop publication \"%s\": %s",
+                                  pubname, PQresultErrorMessage(res));
+               }

6. We should start the standby server before creating the
publications, so that the publications are replicated to standby.
+# 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_p->safe_psql($db1, "SELECT COUNT(*) FROM pg_publication;");

Also maybe we should check the publication count on the standby node
instead of primary.

Thanks and Regards,
Shlok Kyal

Dear Shlok,

4. Instead of warning we should throw an error here:
+       if (PQresultStatus(res) != PGRES_TUPLES_OK)
+       {
+           pg_log_warning("could not obtain publication information: %s",
+                          PQresultErrorMessage(res));
+

I don't think so. ERROR evokes user to retry the command or recreate the physical
replica, but the conversion has already been finished when drop_all_publications()
is called. Cleanup operations should not affect the final result.
drop_primary_replication_slot() and drop_failover_replication_slots() raise WARNING
when they fail to drop objects because they are just cleanup functions.
I feel we can follow this manner.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Wed, 5 Feb 2025 at 07:49, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Dear Shlok,

4. Instead of warning we should throw an error here:
+       if (PQresultStatus(res) != PGRES_TUPLES_OK)
+       {
+           pg_log_warning("could not obtain publication information: %s",
+                          PQresultErrorMessage(res));
+

I don't think so. ERROR evokes user to retry the command or recreate the physical
replica, but the conversion has already been finished when drop_all_publications()
is called. Cleanup operations should not affect the final result.
drop_primary_replication_slot() and drop_failover_replication_slots() raise WARNING
when they fail to drop objects because they are just cleanup functions.
I feel we can follow this manner.

Hi Kuroda-san,

I agree with you. Raising WARNING makes sense to me.

Thanks and Regards,
Shlok Kyal

On Tue, Feb 4, 2025 at 5:39 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:

On Wed, 29 Jan 2025 at 15:14, Shubham Khanna
<khannashubham1197@gmail.com> wrote:

On Wed, Jan 29, 2025 at 10:42 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Dear Shubham,

I propose adding the --clean-publisher-objects option to the
pg_createsubscriber utility. As discussed in [1], this feature ensures
a clean and streamlined setup of logical replication by removing stale
or unnecessary publications from the subscriber node. These
publications, replicated during streaming replication, become
redundant after converting to logical replication and serve no further
purpose. This patch introduces the drop_all_publications() function,
which efficiently fetches and drops all publications on the subscriber
node within a single transaction.

I think replication slot are also type of 'publisher-objects', but they are not
removed for now: API-name may not be accurate. And...

Additionally, other related objects, such as subscriptions and
replication slots, may also require cleanup. I plan to analyze this
further and include them in subsequent patches.

I'm not sure replication slots should be cleaned up. Apart from other items like
publication/subscription, replication slots are not replicated when it is created
on the primary instance. This means they are intentionally created by DBAs and there
may not be no strong reasons to drop them after the conversion.

Another question is the style of APIs. Do you plan to provide APIs like
'cleanup-subscriber-objects' and 'cleanup-publisher-objects', or just one
'cleanup-logical-replication-objects'?

Thanks for the suggestions, I will keep them in mind while preparing
the 0002 patch for the same.
Currently, I have changed the API to '--cleanup-publisher-objects'.

Regarding the patch:

1.
```
+       The <application>pg_createsubscriber</application> now supports the
+       <option>--clean-publisher-objects</option> to remove all publications on
+       the subscriber node before creating a new subscription.
```

This description is not suitable for the documentation. Something like:

Remove all publications on the subscriber node.

2.
```
+       /* Drop publications from the subscriber if requested */
+       drop_all_publications(dbinfo);
```

This should be called when `opts.clean_publisher_objects` is true.

3.
You said publications are dropped within a single transaction, but the
patch does not do. Which is correct?

4.
```
+# Set up node A as primary
+my $node_a = PostgreSQL::Test::Cluster->new('node_a');
+my $aconnstr = $node_a->connstr;
+$node_a->init(allows_streaming => 'logical');
+$node_a->append_conf('postgresql.conf', 'autovacuum = off');
+$node_a->start;
+
+# Set up node B as standby linking to node A
+$node_a->backup('backup_3');
+my $node_b = PostgreSQL::Test::Cluster->new('node_b');
+$node_b->init_from_backup($node_a, 'backup_3', has_streaming => 1);
+$node_b->append_conf(
+       'postgresql.conf', qq[
+       primary_conninfo = '$aconnstr'
+       hot_standby_feedback = on
+       max_logical_replication_workers = 5
+       ]);
+$node_b->set_standby_mode();
+$node_b->start;
```

Fixed the given comments. The attached patch contains the suggested changes.

Hi Shubham,

I have reviewed the v2 patch. Here are my comments:

1. Initial of the comment should in smallcase to make it similar to
other comments:
+ bool cleanup_publisher_objects; /* Drop all publications */

2. We should provide a comment for the function.
+static void
+drop_all_publications(const struct LogicalRepInfo *dbinfo)
+{

3. We should declare it as "const char*"
+   char       *search_query = "SELECT pubname FROM pg_catalog.pg_publication;";
+

4. Instead of warning we should throw an error here:
+       if (PQresultStatus(res) != PGRES_TUPLES_OK)
+       {
+           pg_log_warning("could not obtain publication information: %s",
+                          PQresultErrorMessage(res));
+

5. Should we throw an error in this case as well?
+               if (PQresultStatus(res_for_drop) != PGRES_COMMAND_OK)
+               {
+                   pg_log_warning("could not drop publication \"%s\": %s",
+                                  pubname, PQresultErrorMessage(res));
+               }

6. We should start the standby server before creating the
publications, so that the publications are replicated to standby.
+# 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_p->safe_psql($db1, "SELECT COUNT(*) FROM pg_publication;");

Fixed the given comments. Also, I have kept opting to throw an error
instead of warning as Kuroda suggested in [1]/messages/by-id/OSCPR01MB1496689B1F157DAD598F9E035F5F72@OSCPR01MB14966.jpnprd01.prod.outlook.com.

The attached patch contains the suggested changes.
[1]: /messages/by-id/OSCPR01MB1496689B1F157DAD598F9E035F5F72@OSCPR01MB14966.jpnprd01.prod.outlook.com

Thanks and regards,
Shubham Khanna.

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]/messages/by-id/CAHv8RjL4OvoYafofTb_U_JD5HuyoNowBoGpMfnEbhDSENA74Kg@mail.gmail.com, 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]/messages/by-id/OSCPR01MB1496664FDC38DA40A441F449FF5EE2@OSCPR01MB14966.jpnprd01.prod.outlook.com, 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]https://www.postgresql.org/docs/devel/source-format.html.

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]: /messages/by-id/CAHv8RjL4OvoYafofTb_U_JD5HuyoNowBoGpMfnEbhDSENA74Kg@mail.gmail.com
[2]: /messages/by-id/OSCPR01MB1496664FDC38DA40A441F449FF5EE2@OSCPR01MB14966.jpnprd01.prod.outlook.com
[3]: https://www.postgresql.org/docs/devel/source-format.html

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Fri, Feb 7, 2025 at 7:46 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

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

I have changed the name of the option to "--cleanup-existing-publications"

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

Fixed.

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

Removed this function.

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

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

Fixed.

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?

I agree with you on this. I have removed the drop_all_publication()
and added the "--cleanup-existing-publications" option to the
drop_publication()

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.

Fixed.

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]: /messages/by-id/CAHv8RjL4OvoYafofTb_U_JD5HuyoNowBoGpMfnEbhDSENA74Kg@mail.gmail.com
[2]: /messages/by-id/OSCPR01MB1496664FDC38DA40A441F449FF5EE2@OSCPR01MB14966.jpnprd01.prod.outlook.com
[3]: https://www.postgresql.org/docs/devel/source-format.html

Fixed.

The attached Patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.

On Tue, 11 Feb 2025 at 09:51, Shubham Khanna
<khannashubham1197@gmail.com> wrote:

On Fri, Feb 7, 2025 at 7:46 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

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

I have changed the name of the option to "--cleanup-existing-publications"

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

Fixed.

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

Removed this function.

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

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

Fixed.

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?

I agree with you on this. I have removed the drop_all_publication()
and added the "--cleanup-existing-publications" option to the
drop_publication()

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.

Fixed.

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]: /messages/by-id/CAHv8RjL4OvoYafofTb_U_JD5HuyoNowBoGpMfnEbhDSENA74Kg@mail.gmail.com
[2]: /messages/by-id/OSCPR01MB1496664FDC38DA40A441F449FF5EE2@OSCPR01MB14966.jpnprd01.prod.outlook.com
[3]: https://www.postgresql.org/docs/devel/source-format.html

Fixed.

The attached Patch contains the suggested changes.

Hi Shubham,

I have some comments for v4 patch:
1. I think we should update the comment for the function
'drop_publication'. As its usage is changed with this patch
Currently it states:
/*
* Remove publication if it couldn't finish all steps.
*/

2. In case when --cleanup_existing_publications is not specified the
info message has two double quotes.

pg_createsubscriber: dropping publication
""pg_createsubscriber_5_aa3c31f2"" in database "postgres"

The code:
+       appendPQExpBufferStr(targets,
+                            PQescapeIdentifier(conn, dbinfo->pubname,
+                                               strlen(dbinfo->pubname)));

It is appending the value along with the double quotes. I think we
should assign the value of 'PQescapeIdentifier(conn, dbinfo->pubname,
strlen(dbinfo->pubname)' to a string and then use it.

Thanks and Regards,
Shlok Kyal

Dear Shubham,

Thanks for updating the patch! Few comments.

01. pg_createsubscriber.sgml
```
+      <para>
+       Remove all existing publications on the subscriber node before creating
+       a subscription.
+      </para>
+
```

I think this is not accurate. Publications on databases which are not target will
retain. Also, I'm not sure it is important to clarify "before creating a subscription.".

How about: Remove all existing publications from specified databases.

02. main()
```
+ opt.cleanup_existing_publications = false;
```

ISTM initialization is done with the same ordering with CreateSubscriberOptions.
Thus this should be at after recovery_timeout.

03. 040_pg_createsubscriber.pl
```
+$node_p->wait_for_replay_catchup($node_s);
+
+# 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-existing-publications is run');
```

I think no need to declare $pub_count_before. How about:

```
ok( $node_s->poll_query_until(
$db1, "SELECT COUNT(*) = 2 FROM pg_publication"),
'two publications created before --cleanup-existing-publications is run');
```

04. 040_pg_createsubscriber.pl
``` +my $pub_count_after =
+  $node_s->safe_psql($db1, "SELECT COUNT(*) FROM pg_publication;");
+is($pub_count_after, '0',
+       'all publications dropped after --cleanup-existing-publications is run');
+
```

I think no need to declare $pub_count_after. How about:

```
is($node_s->safe_psql($db1, "SELECT COUNT(*) FROM pg_publication;"), '0',
'all publications dropped after --cleanup-existing-publications is run');
```

05.

According to the document [1]https://www.postgresql.org/docs/devel/error-style-guide.html#ERROR-STYLE-GUIDE-QUOTES, we must do double-quote for each identifiers. But current
patch seems not to do. Below example shows the case when three publications exist.

```
pg_createsubscriber: dropping publications "aaa, bbb, ccc" in database "postgres"
```

I think the output must be `"aaa", "bbb", "ccc"`. This means drop_publication() should be refactored.

[1]: https://www.postgresql.org/docs/devel/error-style-guide.html#ERROR-STYLE-GUIDE-QUOTES

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Tue, Feb 11, 2025 at 9:56 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:

On Tue, 11 Feb 2025 at 09:51, Shubham Khanna
<khannashubham1197@gmail.com> wrote:

On Fri, Feb 7, 2025 at 7:46 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

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

I have changed the name of the option to "--cleanup-existing-publications"

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

Fixed.

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

Removed this function.

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

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

Fixed.

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?

I agree with you on this. I have removed the drop_all_publication()
and added the "--cleanup-existing-publications" option to the
drop_publication()

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.

Fixed.

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]: /messages/by-id/CAHv8RjL4OvoYafofTb_U_JD5HuyoNowBoGpMfnEbhDSENA74Kg@mail.gmail.com
[2]: /messages/by-id/OSCPR01MB1496664FDC38DA40A441F449FF5EE2@OSCPR01MB14966.jpnprd01.prod.outlook.com
[3]: https://www.postgresql.org/docs/devel/source-format.html

Fixed.

The attached Patch contains the suggested changes.

Hi Shubham,

I have some comments for v4 patch:
1. I think we should update the comment for the function
'drop_publication'. As its usage is changed with this patch
Currently it states:
/*
* Remove publication if it couldn't finish all steps.
*/

Fixed.

2. In case when --cleanup_existing_publications is not specified the
info message has two double quotes.

pg_createsubscriber: dropping publication
""pg_createsubscriber_5_aa3c31f2"" in database "postgres"

The code:
+       appendPQExpBufferStr(targets,
+                            PQescapeIdentifier(conn, dbinfo->pubname,
+                                               strlen(dbinfo->pubname)));

It is appending the value along with the double quotes. I think we
should assign the value of 'PQescapeIdentifier(conn, dbinfo->pubname,
strlen(dbinfo->pubname)' to a string and then use it.

Fixed.

The attached patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.

On Wed, Feb 12, 2025 at 12:57 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Dear Shubham,

Thanks for updating the patch! Few comments.

01. pg_createsubscriber.sgml
```
+      <para>
+       Remove all existing publications on the subscriber node before creating
+       a subscription.
+      </para>
+
```

Fixed.

I think this is not accurate. Publications on databases which are not target will
retain. Also, I'm not sure it is important to clarify "before creating a subscription.".

How about: Remove all existing publications from specified databases.

02. main()
```
+ opt.cleanup_existing_publications = false;
```

ISTM initialization is done with the same ordering with CreateSubscriberOptions.
Thus this should be at after recovery_timeout.

Fixed.

03. 040_pg_createsubscriber.pl
```
+$node_p->wait_for_replay_catchup($node_s);
+
+# 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-existing-publications is run');
```

I think no need to declare $pub_count_before. How about:

```
ok( $node_s->poll_query_until(
$db1, "SELECT COUNT(*) = 2 FROM pg_publication"),
'two publications created before --cleanup-existing-publications is run');
```

Fixed.

04. 040_pg_createsubscriber.pl
``` +my $pub_count_after =
+  $node_s->safe_psql($db1, "SELECT COUNT(*) FROM pg_publication;");
+is($pub_count_after, '0',
+       'all publications dropped after --cleanup-existing-publications is run');
+
```

I think no need to declare $pub_count_after. How about:

```
is($node_s->safe_psql($db1, "SELECT COUNT(*) FROM pg_publication;"), '0',
'all publications dropped after --cleanup-existing-publications is run');
```

Fixed.

05.

According to the document [1], we must do double-quote for each identifiers. But current
patch seems not to do. Below example shows the case when three publications exist.

```
pg_createsubscriber: dropping publications "aaa, bbb, ccc" in database "postgres"
```

I think the output must be `"aaa", "bbb", "ccc"`. This means drop_publication() should be refactored.

[1]: https://www.postgresql.org/docs/devel/error-style-guide.html#ERROR-STYLE-GUIDE-QUOTES

Fixed.

The attached patch at [1]/messages/by-id/CAHv8RjKGywu+3cAv9MPgxi1_TUXBT8yzUsW+f=g5UsgeJ8Uk6g@mail.gmail.com contains the suggested changes.

[1]: /messages/by-id/CAHv8RjKGywu+3cAv9MPgxi1_TUXBT8yzUsW+f=g5UsgeJ8Uk6g@mail.gmail.com

Thanks and regards,
Shubham Khanna.

On Thu, 13 Feb 2025 at 15:20, Shubham Khanna
<khannashubham1197@gmail.com> wrote:

On Tue, Feb 11, 2025 at 9:56 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:

On Tue, 11 Feb 2025 at 09:51, Shubham Khanna
<khannashubham1197@gmail.com> wrote:

On Fri, Feb 7, 2025 at 7:46 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

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

I have changed the name of the option to "--cleanup-existing-publications"

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

Fixed.

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

Removed this function.

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

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

Fixed.

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?

I agree with you on this. I have removed the drop_all_publication()
and added the "--cleanup-existing-publications" option to the
drop_publication()

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.

Fixed.

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]: /messages/by-id/CAHv8RjL4OvoYafofTb_U_JD5HuyoNowBoGpMfnEbhDSENA74Kg@mail.gmail.com
[2]: /messages/by-id/OSCPR01MB1496664FDC38DA40A441F449FF5EE2@OSCPR01MB14966.jpnprd01.prod.outlook.com
[3]: https://www.postgresql.org/docs/devel/source-format.html

Fixed.

The attached Patch contains the suggested changes.

Hi Shubham,

I have some comments for v4 patch:
1. I think we should update the comment for the function
'drop_publication'. As its usage is changed with this patch
Currently it states:
/*
* Remove publication if it couldn't finish all steps.
*/

Fixed.

2. In case when --cleanup_existing_publications is not specified the
info message has two double quotes.

pg_createsubscriber: dropping publication
""pg_createsubscriber_5_aa3c31f2"" in database "postgres"

The code:
+       appendPQExpBufferStr(targets,
+                            PQescapeIdentifier(conn, dbinfo->pubname,
+                                               strlen(dbinfo->pubname)));

It is appending the value along with the double quotes. I think we
should assign the value of 'PQescapeIdentifier(conn, dbinfo->pubname,
strlen(dbinfo->pubname)' to a string and then use it.

Fixed.

The attached patch contains the suggested changes.

Hi,

I reviewed v5 patch, I have some comments:

1. I feel that from the description it is not clear from which node we
are removing the publications.
+ Remove all existing publications from specified databases.

Should we write it something like:
Remove all existing publications from specified databases on the target server.

Thoughts?

2. Based on observation in other files, I feel the description can be
in next line:
printf(_("\nOptions:\n"));
+ printf(_(" -c --cleanup-existing-publications drop all
publications on the subscriber\n"));
printf(_(" -d, --database=DBNAME database in which to
create a subscription\n"));

Something like

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

3. Why are we using 'poll_query_until'

+ok( $node_s->poll_query_until(
+ $db1, "SELECT COUNT(*) = 2 FROM pg_publication"),
+ 'two publications created before --cleanup-existing-publications is run');
+

Should we use 'safe_psql'?

Thanks and Regards,
Shlok Kyal

On Thu, Feb 13, 2025 at 5:36 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:

On Thu, 13 Feb 2025 at 15:20, Shubham Khanna
<khannashubham1197@gmail.com> wrote:

On Tue, Feb 11, 2025 at 9:56 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:

On Tue, 11 Feb 2025 at 09:51, Shubham Khanna
<khannashubham1197@gmail.com> wrote:

On Fri, Feb 7, 2025 at 7:46 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

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

I have changed the name of the option to "--cleanup-existing-publications"

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

Fixed.

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

Removed this function.

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

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

Fixed.

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?

I agree with you on this. I have removed the drop_all_publication()
and added the "--cleanup-existing-publications" option to the
drop_publication()

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.

Fixed.

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]: /messages/by-id/CAHv8RjL4OvoYafofTb_U_JD5HuyoNowBoGpMfnEbhDSENA74Kg@mail.gmail.com
[2]: /messages/by-id/OSCPR01MB1496664FDC38DA40A441F449FF5EE2@OSCPR01MB14966.jpnprd01.prod.outlook.com
[3]: https://www.postgresql.org/docs/devel/source-format.html

Fixed.

The attached Patch contains the suggested changes.

Hi Shubham,

I have some comments for v4 patch:
1. I think we should update the comment for the function
'drop_publication'. As its usage is changed with this patch
Currently it states:
/*
* Remove publication if it couldn't finish all steps.
*/

Fixed.

2. In case when --cleanup_existing_publications is not specified the
info message has two double quotes.

pg_createsubscriber: dropping publication
""pg_createsubscriber_5_aa3c31f2"" in database "postgres"

The code:
+       appendPQExpBufferStr(targets,
+                            PQescapeIdentifier(conn, dbinfo->pubname,
+                                               strlen(dbinfo->pubname)));

It is appending the value along with the double quotes. I think we
should assign the value of 'PQescapeIdentifier(conn, dbinfo->pubname,
strlen(dbinfo->pubname)' to a string and then use it.

Fixed.

The attached patch contains the suggested changes.

Hi,

I reviewed v5 patch, I have some comments:

1. I feel that from the description it is not clear from which node we
are removing the publications.
+ Remove all existing publications from specified databases.

Should we write it something like:
Remove all existing publications from specified databases on the target server.

Thoughts?

2. Based on observation in other files, I feel the description can be
in next line:
printf(_("\nOptions:\n"));
+ printf(_(" -c --cleanup-existing-publications drop all
publications on the subscriber\n"));
printf(_(" -d, --database=DBNAME database in which to
create a subscription\n"));

Something like

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

3. Why are we using 'poll_query_until'

+ok( $node_s->poll_query_until(
+ $db1, "SELECT COUNT(*) = 2 FROM pg_publication"),
+ 'two publications created before --cleanup-existing-publications is run');
+

Should we use 'safe_psql'?

Fixed the given comments. The attached patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.

On Fri, 14 Feb 2025 at 10:50, Shubham Khanna
<khannashubham1197@gmail.com> wrote:

On Thu, Feb 13, 2025 at 5:36 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:

On Thu, 13 Feb 2025 at 15:20, Shubham Khanna
<khannashubham1197@gmail.com> wrote:

On Tue, Feb 11, 2025 at 9:56 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:

On Tue, 11 Feb 2025 at 09:51, Shubham Khanna
<khannashubham1197@gmail.com> wrote:

On Fri, Feb 7, 2025 at 7:46 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

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

I have changed the name of the option to "--cleanup-existing-publications"

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

Fixed.

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

Removed this function.

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

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

Fixed.

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?

I agree with you on this. I have removed the drop_all_publication()
and added the "--cleanup-existing-publications" option to the
drop_publication()

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.

Fixed.

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]: /messages/by-id/CAHv8RjL4OvoYafofTb_U_JD5HuyoNowBoGpMfnEbhDSENA74Kg@mail.gmail.com
[2]: /messages/by-id/OSCPR01MB1496664FDC38DA40A441F449FF5EE2@OSCPR01MB14966.jpnprd01.prod.outlook.com
[3]: https://www.postgresql.org/docs/devel/source-format.html

Fixed.

The attached Patch contains the suggested changes.

Hi Shubham,

I have some comments for v4 patch:
1. I think we should update the comment for the function
'drop_publication'. As its usage is changed with this patch
Currently it states:
/*
* Remove publication if it couldn't finish all steps.
*/

Fixed.

2. In case when --cleanup_existing_publications is not specified the
info message has two double quotes.

pg_createsubscriber: dropping publication
""pg_createsubscriber_5_aa3c31f2"" in database "postgres"

The code:
+       appendPQExpBufferStr(targets,
+                            PQescapeIdentifier(conn, dbinfo->pubname,
+                                               strlen(dbinfo->pubname)));

It is appending the value along with the double quotes. I think we
should assign the value of 'PQescapeIdentifier(conn, dbinfo->pubname,
strlen(dbinfo->pubname)' to a string and then use it.

Fixed.

The attached patch contains the suggested changes.

Hi,

I reviewed v5 patch, I have some comments:

1. I feel that from the description it is not clear from which node we
are removing the publications.
+ Remove all existing publications from specified databases.

Should we write it something like:
Remove all existing publications from specified databases on the target server.

Thoughts?

2. Based on observation in other files, I feel the description can be
in next line:
printf(_("\nOptions:\n"));
+ printf(_(" -c --cleanup-existing-publications drop all
publications on the subscriber\n"));
printf(_(" -d, --database=DBNAME database in which to
create a subscription\n"));

Something like

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

3. Why are we using 'poll_query_until'

+ok( $node_s->poll_query_until(
+ $db1, "SELECT COUNT(*) = 2 FROM pg_publication"),
+ 'two publications created before --cleanup-existing-publications is run');
+

Should we use 'safe_psql'?

Fixed the given comments. The attached patch contains the suggested changes.

Hi Shubham,

I reviewed the v6 patch and here are some of my comments.

1. I think we do not need 'search_query' variable. We can directly
combine it in PQexec.
+       const char *search_query =
+           "SELECT pubname FROM pg_catalog.pg_publication;";

-   pg_log_info("dropping publication \"%s\" in database \"%s\"",
-               dbinfo->pubname, dbinfo->dbname);
+       res = PQexec(conn, search_query);

2.
+ * Remove all existing publications from specified databases on the target
+ * server.

The previous comment in v5 for function 'drop_publication' was more
appropriate. I think you mistakenly changed it here.
I suggested changing it in pg_createsubscriber.sgml in point 1 of [1]/messages/by-id/CANhcyEUo7F954LULk859xs6FtwQ5USH6C2tiBbGwpihU2yHmAQ@mail.gmail.com.

[1]: /messages/by-id/CANhcyEUo7F954LULk859xs6FtwQ5USH6C2tiBbGwpihU2yHmAQ@mail.gmail.com

Thanks and Regards,
Shlok Kyal

On Fri, Feb 14, 2025 at 4:57 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:

On Fri, 14 Feb 2025 at 10:50, Shubham Khanna
<khannashubham1197@gmail.com> wrote:

On Thu, Feb 13, 2025 at 5:36 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:

On Thu, 13 Feb 2025 at 15:20, Shubham Khanna
<khannashubham1197@gmail.com> wrote:

On Tue, Feb 11, 2025 at 9:56 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:

On Tue, 11 Feb 2025 at 09:51, Shubham Khanna
<khannashubham1197@gmail.com> wrote:

On Fri, Feb 7, 2025 at 7:46 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

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

I have changed the name of the option to "--cleanup-existing-publications"

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

Fixed.

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

Removed this function.

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

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

Fixed.

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?

I agree with you on this. I have removed the drop_all_publication()
and added the "--cleanup-existing-publications" option to the
drop_publication()

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.

Fixed.

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]: /messages/by-id/CAHv8RjL4OvoYafofTb_U_JD5HuyoNowBoGpMfnEbhDSENA74Kg@mail.gmail.com
[2]: /messages/by-id/OSCPR01MB1496664FDC38DA40A441F449FF5EE2@OSCPR01MB14966.jpnprd01.prod.outlook.com
[3]: https://www.postgresql.org/docs/devel/source-format.html

Fixed.

The attached Patch contains the suggested changes.

Hi Shubham,

I have some comments for v4 patch:
1. I think we should update the comment for the function
'drop_publication'. As its usage is changed with this patch
Currently it states:
/*
* Remove publication if it couldn't finish all steps.
*/

Fixed.

2. In case when --cleanup_existing_publications is not specified the
info message has two double quotes.

pg_createsubscriber: dropping publication
""pg_createsubscriber_5_aa3c31f2"" in database "postgres"

The code:
+       appendPQExpBufferStr(targets,
+                            PQescapeIdentifier(conn, dbinfo->pubname,
+                                               strlen(dbinfo->pubname)));

It is appending the value along with the double quotes. I think we
should assign the value of 'PQescapeIdentifier(conn, dbinfo->pubname,
strlen(dbinfo->pubname)' to a string and then use it.

Fixed.

The attached patch contains the suggested changes.

Hi,

I reviewed v5 patch, I have some comments:

1. I feel that from the description it is not clear from which node we
are removing the publications.
+ Remove all existing publications from specified databases.

Should we write it something like:
Remove all existing publications from specified databases on the target server.

Thoughts?

2. Based on observation in other files, I feel the description can be
in next line:
printf(_("\nOptions:\n"));
+ printf(_(" -c --cleanup-existing-publications drop all
publications on the subscriber\n"));
printf(_(" -d, --database=DBNAME database in which to
create a subscription\n"));

Something like

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

3. Why are we using 'poll_query_until'

+ok( $node_s->poll_query_until(
+ $db1, "SELECT COUNT(*) = 2 FROM pg_publication"),
+ 'two publications created before --cleanup-existing-publications is run');
+

Should we use 'safe_psql'?

Fixed the given comments. The attached patch contains the suggested changes.

Hi Shubham,

I reviewed the v6 patch and here are some of my comments.

1. I think we do not need 'search_query' variable. We can directly
combine it in PQexec.
+       const char *search_query =
+           "SELECT pubname FROM pg_catalog.pg_publication;";

-   pg_log_info("dropping publication \"%s\" in database \"%s\"",
-               dbinfo->pubname, dbinfo->dbname);
+       res = PQexec(conn, search_query);

2.
+ * Remove all existing publications from specified databases on the target
+ * server.

The previous comment in v5 for function 'drop_publication' was more
appropriate. I think you mistakenly changed it here.
I suggested changing it in pg_createsubscriber.sgml in point 1 of [1].

[1]: /messages/by-id/CANhcyEUo7F954LULk859xs6FtwQ5USH6C2tiBbGwpihU2yHmAQ@mail.gmail.com

Fixed the given comments. The attached patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.

Hi Shubham

Some review comments for v7-0001.

(I am late to this thread. If some of my comments have already been
discussed and rejected please let me know).

======
1 GENERAL. Option Name?

Wondering why the patch is introducing more terminology like
"cleanup"; if we are dropping publications then why not say "drop"?
Also I am not sure if "existing" means anything because you cannot
cleanup/drop something that is not "existing".

IOW, why not call this the "--drop-publications" option?

======
Commit message

2.
These publications, replicated during streaming replication, become redundant
after converting to logical replication and serve no further purpose.

~

From this description it seems there is an assumption that the only
publications on the target server are those that were physically
replicated to the standby. Is that strictly true? Isn't it also
possible that a user might have created their own publication on the
target server prior to running the pg_createsubscriber. So even if
they want all the physically replicated ones to be removed, they would
NOT want their own new publication to also get removed at the same
time.

E.g. The original motivation thread [1]/messages/by-id/CAExHW5t4ew7ZrgcDdTv7YmuG7LVQT1ZaEny_EvtngHtEBNyjcQ@mail.gmail.com for this patch only said "But
what good is having the SAME publications as primary also on logical
replica?" (my emphasis of "same") so it seems there should be some
sort of name matching before just dropping everything.

Actually.... The code looks like it might be doing the correct thing
already and only fetching the publication names from the source server
and then deleting only those names from the target server. But this
comment message didn't describe this clearly.

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

3.
+      <para>
+       Remove all existing publications from specified databases on tne target
+       server.
+      </para>

typo "tne"

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

struct CreateSubscriberOptions:

4.
+ bool cleanup_existing_publications; /* drop all publications */

The field name seems overkill. e.g. As mentioned for the option, it
could be called 'drop' instead of cleanup. And the 'existing' seems
superfluous because you can only drop something that exists. So why
not just 'drop_publications'. Won't that have the same meaning?

~~~

5.
 static void
-setup_subscriber(struct LogicalRepInfo *dbinfo, const char *consistent_lsn)
+setup_subscriber(struct LogicalRepInfo *dbinfo, const char *consistent_lsn,
+ bool cleanup_existing_publications)

The setup_subscriber's function comment does not say anything about
this function potentially also dropping publications at the
subscriber.

~~~

6.
 static void
-drop_publication(PGconn *conn, struct LogicalRepInfo *dbinfo)
+drop_publication(PGconn *conn, struct LogicalRepInfo *dbinfo,
+ bool cleanup_existing_publications)

6a.
This arrangement seems complicated to me.

IMO it would be more natural to have 2 separate functions, and just
call the appropriate one.
drop_publication()
drop_all_publications()

instead of trying to make this function do everything.

~

6b.
Furthermore, can't you just have a common helper function to DROP a
single PUBLICATION by name?

Then the code that drops all publications can just loop to call this
common dropper for each iteration. Code should be much simpler. I
don't see the efficiency of this operation is really a factor,
pg_createsubscriber is rarely used, so IMO a better goal is code
simplicity/maintenance.

e.g. drop_publication() --> _drop_one_publication()
e.g drop_all_publications() --> LOOP (pub list) { _drop_one_publication() }

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

7.
+
+# 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;");
+

/it's/their/

~~~

8.
Maybe also do a CREATE PUBLICATION at node_s, prior to the
pg_createsubvscript, so then you can verify that the user-created one
is unaffected by the cleanup of all the others.

======
[1]: /messages/by-id/CAExHW5t4ew7ZrgcDdTv7YmuG7LVQT1ZaEny_EvtngHtEBNyjcQ@mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

On Mon, 17 Feb 2025 at 09:49, Peter Smith <smithpb2250@gmail.com> wrote:

Hi Shubham

Some review comments for v7-0001.

(I am late to this thread. If some of my comments have already been
discussed and rejected please let me know).

======
1 GENERAL. Option Name?

Wondering why the patch is introducing more terminology like
"cleanup"; if we are dropping publications then why not say "drop"?
Also I am not sure if "existing" means anything because you cannot
cleanup/drop something that is not "existing".

IOW, why not call this the "--drop-publications" option?

We should keep this option generic as this same option should be
enhanced further to handle cleaning other objects which was suggested
earlier at [1]/messages/by-id/CAExHW5t4ew7ZrgcDdTv7YmuG7LVQT1ZaEny_EvtngHtEBNyjcQ@mail.gmail.com.
How about something like:
remove-non-logical-objects/clean-non-logical-objects/purge-non-replicated/discard-non-replicated
or something better?

[1]: /messages/by-id/CAExHW5t4ew7ZrgcDdTv7YmuG7LVQT1ZaEny_EvtngHtEBNyjcQ@mail.gmail.com

Regards,
Vignesh

On Mon, Feb 17, 2025 at 9:49 AM Peter Smith <smithpb2250@gmail.com> wrote:

Hi Shubham

Some review comments for v7-0001.

(I am late to this thread. If some of my comments have already been
discussed and rejected please let me know).

======
1 GENERAL. Option Name?

Wondering why the patch is introducing more terminology like
"cleanup"; if we are dropping publications then why not say "drop"?
Also I am not sure if "existing" means anything because you cannot
cleanup/drop something that is not "existing".

IOW, why not call this the "--drop-publications" option?

I have retained the option name as '--cleanup-existing-publications'
for now. However, I understand the concern regarding terminology and
will revise it in the next patch version once there is a consensus on
the final name.

======
Commit message

2.
These publications, replicated during streaming replication, become redundant
after converting to logical replication and serve no further purpose.

~

From this description it seems there is an assumption that the only
publications on the target server are those that were physically
replicated to the standby. Is that strictly true? Isn't it also
possible that a user might have created their own publication on the
target server prior to running the pg_createsubscriber. So even if
they want all the physically replicated ones to be removed, they would
NOT want their own new publication to also get removed at the same
time.

E.g. The original motivation thread [1] for this patch only said "But
what good is having the SAME publications as primary also on logical
replica?" (my emphasis of "same") so it seems there should be some
sort of name matching before just dropping everything.

Actually.... The code looks like it might be doing the correct thing
already and only fetching the publication names from the source server
and then deleting only those names from the target server. But this
comment message didn't describe this clearly.

======

Modified the commit message.

doc/src/sgml/ref/pg_createsubscriber.sgml

3.
+      <para>
+       Remove all existing publications from specified databases on tne target
+       server.
+      </para>

typo "tne"

======

Fixed.

src/bin/pg_basebackup/pg_createsubscriber.c

struct CreateSubscriberOptions:

4.
+ bool cleanup_existing_publications; /* drop all publications */

The field name seems overkill. e.g. As mentioned for the option, it
could be called 'drop' instead of cleanup. And the 'existing' seems
superfluous because you can only drop something that exists. So why
not just 'drop_publications'. Won't that have the same meaning?

~~~

Fixed.

5.
static void
-setup_subscriber(struct LogicalRepInfo *dbinfo, const char *consistent_lsn)
+setup_subscriber(struct LogicalRepInfo *dbinfo, const char *consistent_lsn,
+ bool cleanup_existing_publications)

The setup_subscriber's function comment does not say anything about
this function potentially also dropping publications at the
subscriber.

~~~

Fixed.

6.
static void
-drop_publication(PGconn *conn, struct LogicalRepInfo *dbinfo)
+drop_publication(PGconn *conn, struct LogicalRepInfo *dbinfo,
+ bool cleanup_existing_publications)

6a.
This arrangement seems complicated to me.

IMO it would be more natural to have 2 separate functions, and just
call the appropriate one.
drop_publication()
drop_all_publications()

instead of trying to make this function do everything.

~

6b.
Furthermore, can't you just have a common helper function to DROP a
single PUBLICATION by name?

Then the code that drops all publications can just loop to call this
common dropper for each iteration. Code should be much simpler. I
don't see the efficiency of this operation is really a factor,
pg_createsubscriber is rarely used, so IMO a better goal is code
simplicity/maintenance.

e.g. drop_publication() --> _drop_one_publication()
e.g drop_all_publications() --> LOOP (pub list) { _drop_one_publication() }

======

Fixed.

.../t/040_pg_createsubscriber.pl

7.
+
+# 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;");
+

/it's/their/

~~~

Fixed.

8.
Maybe also do a CREATE PUBLICATION at node_s, prior to the
pg_createsubvscript, so then you can verify that the user-created one
is unaffected by the cleanup of all the others.

======

Since $node_s is a streaming standby, it does not allow object
creation. As a result, publications cannot be created on $node_s.

[1] /messages/by-id/CAExHW5t4ew7ZrgcDdTv7YmuG7LVQT1ZaEny_EvtngHtEBNyjcQ@mail.gmail.com

The attached patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.