Thread: RE: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
RE: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
"Hayato Kuroda (Fujitsu)"
Date:
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
Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
Shlok Kyal
Date:
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
Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
Shlok Kyal
Date:
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
RE: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
"Hayato Kuroda (Fujitsu)"
Date:
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
Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
Shlok Kyal
Date:
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]: 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
> >
>
> 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
RE: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
"Hayato Kuroda (Fujitsu)"
Date:
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], 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
Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
Shubham Khanna
Date:
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] contains the suggested changes. [1] - https://www.postgresql.org/message-id/CAHv8RjKGywu%2B3cAv9MPgxi1_TUXBT8yzUsW%2Bf%3Dg5UsgeJ8Uk6g%40mail.gmail.com Thanks and regards, Shubham Khanna.
Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
Shlok Kyal
Date:
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]: 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
> > > >
> > >
> > > 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
Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
Shlok Kyal
Date:
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]:
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
> > > > > >
> > > > >
> > > > > 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]: https://www.postgresql.org/message-id/CANhcyEUo7F954LULk859xs6FtwQ5USH6C2tiBbGwpihU2yHmAQ%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
Peter Smith
Date:
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] 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] https://www.postgresql.org/message-id/CAExHW5t4ew7ZrgcDdTv7YmuG7LVQT1ZaEny_EvtngHtEBNyjcQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
vignesh C
Date:
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]. How about something like: remove-non-logical-objects/clean-non-logical-objects/purge-non-replicated/discard-non-replicated or something better? [1] - https://www.postgresql.org/message-id/CAExHW5t4ew7ZrgcDdTv7YmuG7LVQT1ZaEny_EvtngHtEBNyjcQ%40mail.gmail.com Regards, Vignesh
Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
Shubham Khanna
Date:
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] https://www.postgresql.org/message-id/CAExHW5t4ew7ZrgcDdTv7YmuG7LVQT1ZaEny_EvtngHtEBNyjcQ%40mail.gmail.com
>
The attached patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
Attachment
Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
Peter Smith
Date:
On Thu, Feb 20, 2025 at 5:44 PM Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> On Mon, Feb 17, 2025 at 9:49 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
...
> > ======
> > 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.
>
BTW, Is it even correct to assume that the user has to choose between
(a) clean everything or (b) clean nothing? That is why I felt
something that mentions only ‘publication’ gives more flexibility.
Later, when some DROP <other_stuff> feature might be added then we can
make additional --drop-XXX switches, or a --drop-all switch for
cleaning everything
//////
Some more review comments for patch v8-0001
======
Commit message
1.
To prevent accidental removal of user-created publications on the subscriber,
this cleanup process only targets publications that existed on the source
server and were replicated to the subscriber.
Any manually created publications on the subscriber will remain intact.
~
Is this scenario (manually created publications on the subscriber)
possible to do?
If yes, then there should be a test case for it
If not, then maybe this whole paragraph needs rewording.
~~~
2.
This feature is optional and only takes effect when the
'--cleanup-existing-publications' option is explicitly specified.
Since publication cleanup is not required when upgrading streaming replication
clusters, this option provides users with the flexibility to enable or skip the
cleanup process as needed.
~
AFAICT, this is pretty much just saying that the switch is optional
and that the feature only does something when the switch is specified
by the user. But, doesn't all that just go without saying?
======
src/bin/pg_basebackup/pg_createsubscriber.c
3.
+ /* Drop all existing publications if requested */
+ if (drop_publications)
+ {
+ pg_log_info("Dropping all existing publications in database \"%s\"",
+ dbinfo[i].dbname);
+ drop_all_publications(conn, dbinfo[i].dbname);
+ }
+
/*
* Since the publication was created before the consistent LSN, it is
* available on the subscriber when the physical replica is promoted.
* Remove publications from the subscriber because it has no use.
+ * Additionally, drop publications existed before this command if
+ * requested.
*/
drop_publication(conn, &dbinfo[i]);
~
3a.
The logic of this code seems strange - e.g. it is optionally dropping
all publications, and then dropping yet another one again. Won't that
be already dropped as part of the drop_all?
~
3b.
Anyway, perhaps the logic should be encapsulated in a new helper
check_and_drop_existing_publications() function to match the existing
'check_and_drop_existing_subscriptions()
~
3c.
I felt logs like "Dropping all existing publications in database
\"%s\"" should be logged within the function drop_all_publications,
not at the caller.
~~~
_drop_one_publication:
4.
-/*
- * Remove publication if it couldn't finish all steps.
- */
+/* Helper function to drop a single publication by name. */
static void
-drop_publication(PGconn *conn, struct LogicalRepInfo *dbinfo)
+_drop_one_publication(PGconn *conn, const char *pubname, const char *dbname)
A better name for this helper might be drop_publication_by_name()
~~~
5.
+ appendPQExpBuffer(query, "DROP PUBLICATION %s", pubname);
+ pg_log_debug("Executing: %s", query->data);
+ pg_log_info("Dropping publication %s in database \"%s\"", pubname,
dbinfo->dbname);
That debug seems more typically written as:
pg_log_debug("command is: %s", query->data);
~~~
drop_all_publications:
6.
+/* Drop all publications in the database. */
+static void
+drop_all_publications(PGconn *conn, const char *dbname)
+{
+ PGresult *res;
+ int count = 0;
The 'count' variable seems unused.
~~~
7.
+ for (int i = 0; i < PQntuples(res); i++)
+ {
+ char *pubname_esc = PQescapeIdentifier(conn, PQgetvalue(res, i, 0),
+ strlen(PQgetvalue(res, i, 0)));
+
+ _drop_one_publication(conn, pubname_esc, dbname);
+ PQfreemem(pubname_esc);
+ count++;
+ }
Instead of escaping the name here, and also escaping it in
drop_publication(), that could be done inside the common helper
function.
======
.../t/040_pg_createsubscriber.pl
8.
+# Create publications to test their 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;");
8a.
If you are going to number the publications then it would be better to
number all of them so there is a consistent naming.
e.g. test_pub1,test_pub2; instead of test_pub,test_pub2
~
8b.
Should share the same safe_psql for all DDL when it is possible.
~
8c.
Maybe the comment should give a bit more explanation of what is going
on here. e.g. this is preparation for the following
--cleanup-existing-publications test.
Something that conveys the following (choose your own wording):
Create some user-defined publications. After waiting for the streaming
replication to replicate these to the standby, we can use the
pg_createsubscriber to check that the --cleanup-existing-publications
switch causes them to be removed.
~~~
9.
I wonder if you need 2 test cases
i) use --cleanup-existing-publications and verify publication are removed
ii) DON'T use --cleanup-existing-publications and verify publication still exist
======
Kind Regards,
Peter Smith.
Fujitsu Australia
RE: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
"Hayato Kuroda (Fujitsu)"
Date:
Dear Shubham,
Thanks for updating the patch. Here are comments.
01.
```
/*
* Create the subscriptions, adjust the initial location for logical
* replication and enable the subscriptions. That's the last step for logical
* replication setup. If 'drop_publications' is true, existing publications on
* the subscriber will be dropped before creating new subscriptions.
*/
static void
setup_subscriber(struct LogicalRepInfo *dbinfo, const char *consistent_lsn,
bool drop_publications)
```
Even drop_publications is false, at least one publication would be dropped. The
argument is not correct. I prefer previous name.
02.
```
/* Drop all existing publications if requested */
if (drop_publications)
{
pg_log_info("Dropping all existing publications in database \"%s\"",
dbinfo[i].dbname);
drop_all_publications(conn, dbinfo[i].dbname);
}
/*
* Since the publication was created before the consistent LSN, it is
* available on the subscriber when the physical replica is promoted.
* Remove publications from the subscriber because it has no use.
* Additionally, drop publications existed before this command if
* requested.
*/
drop_publication(conn, &dbinfo[i]);
```
It is quite strange that drop_publication() is called even when drop_all_publications() is called.
03.
Also, I'm not sure pg_log_info() is needed here. _drop_one_publication() outputs dropping publications.
04.
```
/* Helper function to drop a single publication by name. */
static void
_drop_one_publication(PGconn *conn, const char *pubname, const char *dbname)
```
Functions recently added do not have prefix "_". I think it can be removed.
05.
```
pg_log_debug("Executing: %s", query->data);
pg_log_info("Dropping publication %s in database \"%s\"", pubname, dbinfo->dbname);
```
In _drop_one_publication(), dbname is used only for the message. Can we move to pg_log_info()
outside the function and reduce the argument?
06.
Also, please start with lower case.
07.
Also, please preserve the message as much as possible. Previously they are:
```
pg_log_info("dropping publication \"%s\" in database \"%s\"", dbinfo->pubname, dbinfo->dbname);
pg_log_debug("command is: %s", str->data);
```
08.
I feel we must update made_publication.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
Shubham Khanna
Date:
On Fri, Feb 21, 2025 at 8:31 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Thu, Feb 20, 2025 at 5:44 PM Shubham Khanna
> <khannashubham1197@gmail.com> wrote:
> >
> > On Mon, Feb 17, 2025 at 9:49 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> ...
> > > ======
> > > 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.
> >
>
> BTW, Is it even correct to assume that the user has to choose between
> (a) clean everything or (b) clean nothing? That is why I felt
> something that mentions only ‘publication’ gives more flexibility.
> Later, when some DROP <other_stuff> feature might be added then we can
> make additional --drop-XXX switches, or a --drop-all switch for
> cleaning everything
>
> //////
>
> Some more review comments for patch v8-0001
>
> ======
> Commit message
>
> 1.
> To prevent accidental removal of user-created publications on the subscriber,
> this cleanup process only targets publications that existed on the source
> server and were replicated to the subscriber.
> Any manually created publications on the subscriber will remain intact.
>
> ~
>
> Is this scenario (manually created publications on the subscriber)
> possible to do?
> If yes, then there should be a test case for it
> If not, then maybe this whole paragraph needs rewording.
>
> ~~~
Fixed.
>
> 2.
> This feature is optional and only takes effect when the
> '--cleanup-existing-publications' option is explicitly specified.
> Since publication cleanup is not required when upgrading streaming replication
> clusters, this option provides users with the flexibility to enable or skip the
> cleanup process as needed.
>
> ~
>
> AFAICT, this is pretty much just saying that the switch is optional
> and that the feature only does something when the switch is specified
> by the user. But, doesn't all that just go without saying?
>
>
Fixed.
> ======
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> 3.
> + /* Drop all existing publications if requested */
> + if (drop_publications)
> + {
> + pg_log_info("Dropping all existing publications in database \"%s\"",
> + dbinfo[i].dbname);
> + drop_all_publications(conn, dbinfo[i].dbname);
> + }
> +
> /*
> * Since the publication was created before the consistent LSN, it is
> * available on the subscriber when the physical replica is promoted.
> * Remove publications from the subscriber because it has no use.
> + * Additionally, drop publications existed before this command if
> + * requested.
> */
> drop_publication(conn, &dbinfo[i]);
> ~
>
> 3a.
> The logic of this code seems strange - e.g. it is optionally dropping
> all publications, and then dropping yet another one again. Won't that
> be already dropped as part of the drop_all?
>
> ~
>
Fixed.
> 3b.
> Anyway, perhaps the logic should be encapsulated in a new helper
> check_and_drop_existing_publications() function to match the existing
> 'check_and_drop_existing_subscriptions()
>
> ~
>
Added a new function 'check_and_drop_exixsting_publications()'
> 3c.
> I felt logs like "Dropping all existing publications in database
> \"%s\"" should be logged within the function drop_all_publications,
> not at the caller.
>
> ~~~
Fixed.
>
> _drop_one_publication:
>
> 4.
> -/*
> - * Remove publication if it couldn't finish all steps.
> - */
> +/* Helper function to drop a single publication by name. */
> static void
> -drop_publication(PGconn *conn, struct LogicalRepInfo *dbinfo)
> +_drop_one_publication(PGconn *conn, const char *pubname, const char *dbname)
>
> A better name for this helper might be drop_publication_by_name()
>
> ~~~
Removed this function.
>
> 5.
> + appendPQExpBuffer(query, "DROP PUBLICATION %s", pubname);
> + pg_log_debug("Executing: %s", query->data);
> + pg_log_info("Dropping publication %s in database \"%s\"", pubname,
> dbinfo->dbname);
>
> That debug seems more typically written as:
> pg_log_debug("command is: %s", query->data);
>
> ~~~
Fixed.
>
> drop_all_publications:
>
> 6.
> +/* Drop all publications in the database. */
> +static void
> +drop_all_publications(PGconn *conn, const char *dbname)
> +{
> + PGresult *res;
> + int count = 0;
>
> The 'count' variable seems unused.
>
> ~~~
Removed the usage of 'count'.
>
> 7.
> + for (int i = 0; i < PQntuples(res); i++)
> + {
> + char *pubname_esc = PQescapeIdentifier(conn, PQgetvalue(res, i, 0),
> + strlen(PQgetvalue(res, i, 0)));
> +
> + _drop_one_publication(conn, pubname_esc, dbname);
> + PQfreemem(pubname_esc);
> + count++;
> + }
>
> Instead of escaping the name here, and also escaping it in
> drop_publication(), that could be done inside the common helper
> function.
>
Fixed.
> ======
> .../t/040_pg_createsubscriber.pl
>
> 8.
> +# Create publications to test their 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;");
>
> 8a.
> If you are going to number the publications then it would be better to
> number all of them so there is a consistent naming.
>
> e.g. test_pub1,test_pub2; instead of test_pub,test_pub2
>
> ~
Fixed.
>
> 8b.
> Should share the same safe_psql for all DDL when it is possible.
>
> ~
Fixed.
>
> 8c.
> Maybe the comment should give a bit more explanation of what is going
> on here. e.g. this is preparation for the following
> --cleanup-existing-publications test.
>
> Something that conveys the following (choose your own wording):
>
> Create some user-defined publications. After waiting for the streaming
> replication to replicate these to the standby, we can use the
> pg_createsubscriber to check that the --cleanup-existing-publications
> switch causes them to be removed.
>
> ~~~
Fixed.
>
> 9.
> I wonder if you need 2 test cases
> i) use --cleanup-existing-publications and verify publication are removed
> ii) DON'T use --cleanup-existing-publications and verify publication still exist
Added a new test case where the option is not being used and the
verification of the user created publications is done. I have created
two new nodes for this test case.
The attached patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
Attachment
Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
Shubham Khanna
Date:
On Fri, Feb 21, 2025 at 3:06 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shubham,
>
> Thanks for updating the patch. Here are comments.
>
> 01.
> ```
> /*
> * Create the subscriptions, adjust the initial location for logical
> * replication and enable the subscriptions. That's the last step for logical
> * replication setup. If 'drop_publications' is true, existing publications on
> * the subscriber will be dropped before creating new subscriptions.
> */
> static void
> setup_subscriber(struct LogicalRepInfo *dbinfo, const char *consistent_lsn,
> bool drop_publications)
> ```
>
> Even drop_publications is false, at least one publication would be dropped. The
> argument is not correct. I prefer previous name.
>
Fixed.
> 02.
> ```
> /* Drop all existing publications if requested */
> if (drop_publications)
> {
> pg_log_info("Dropping all existing publications in database \"%s\"",
> dbinfo[i].dbname);
> drop_all_publications(conn, dbinfo[i].dbname);
> }
>
> /*
> * Since the publication was created before the consistent LSN, it is
> * available on the subscriber when the physical replica is promoted.
> * Remove publications from the subscriber because it has no use.
> * Additionally, drop publications existed before this command if
> * requested.
> */
> drop_publication(conn, &dbinfo[i]);
> ```
>
> It is quite strange that drop_publication() is called even when drop_all_publications() is called.
>
Fixed.
> 03.
> Also, I'm not sure pg_log_info() is needed here. _drop_one_publication() outputs dropping publications.
>
Fixed.
> 04.
> ```
> /* Helper function to drop a single publication by name. */
> static void
> _drop_one_publication(PGconn *conn, const char *pubname, const char *dbname)
> ```
>
> Functions recently added do not have prefix "_". I think it can be removed.
>
Removed this function.
> 05.
> ```
> pg_log_debug("Executing: %s", query->data);
> pg_log_info("Dropping publication %s in database \"%s\"", pubname, dbinfo->dbname);
> ```
> In _drop_one_publication(), dbname is used only for the message. Can we move to pg_log_info()
> outside the function and reduce the argument?
>
Fixed.
> 06.
> Also, please start with lower case.
>
Fixed.
> 07.
> Also, please preserve the message as much as possible. Previously they are:
> ```
> pg_log_info("dropping publication \"%s\" in database \"%s\"", dbinfo->pubname, dbinfo->dbname);
> pg_log_debug("command is: %s", str->data);
> ```
>
Fixed.
> 08.
> I feel we must update made_publication.
>
Fixed.
The attached patch at [1] contains the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2BzmkwunNubo%2B_Gp26S_qXbD%3Dp%2BrMfEnPnjiEE1A6GTXQ%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
RE: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
"Hayato Kuroda (Fujitsu)"
Date:
Dear Shubham,
Thanks for updating the patch. Few comments.
01. CreateSubscriberOptions
```
+ bool cleanup_existing_publications; /* drop all publications */
```
My previous comment [1] #1 did not intend to update attributes. My point was
only for the third argument of setup_subscriber().
02. setup_subscriber()
```
+ pg_log_info("cleaning up existing publications on the subscriber");
```
I don't think this output is needed. check_and_drop_existing_publications() has the similar output.
03. drop_publication_by_name()
```
+
+ appendPQExpBuffer(query, "DROP PUBLICATION %s", pubname_esc);
+ pg_log_info("dropping publication %s in database \"%s\"", pubname, dbname);
+ pg_log_debug("command is: %s", query->data);
```
Currently, appendPQExpBuffer() is done after the pg_log_info(). Please reserve current ordering if
there are no reasons. Also, variable "str" is currently used instead of query, please follow.
04. drop_publication_by_name()
```
if (!dry_run)
{
- res = PQexec(conn, str->data);
+ res = PQexec(conn, query->data);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
+ dbinfo->made_publication = false;
+ else
{
- 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. */
-
- /*
- * Don't disconnect and exit here. This routine is used by primary
- * (cleanup publication / replication slot due to an error) and
- * subscriber (remove the replicated publications). In both cases,
- * it can continue and provide instructions for the user to remove
- * it later if cleanup fails.
- */
+ pg_log_error("could not drop publication %s in database \"%s\": %s",
+ pubname, dbname, PQresultErrorMessage(res));
}
```
pg_log_error() is exected when the command succeed: This is not correct.
05. 040_pg_createsubscriber.pl
```
+# 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;
```
I don't think new primary is not needed. Can't you reuse node_p?
[1]:
https://www.postgresql.org/message-id/OSCPR01MB14966D845AC77FC46ECEECCE4F5C72%40OSCPR01MB14966.jpnprd01.prod.outlook.com
Best regards,
Hayato Kuroda
FUJITSU LIMITED
Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
Shubham Khanna
Date:
On Tue, Feb 25, 2025 at 4:50 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shubham,
>
> Thanks for updating the patch. Few comments.
>
> 01. CreateSubscriberOptions
> ```
> + bool cleanup_existing_publications; /* drop all publications */
> ```
>
> My previous comment [1] #1 did not intend to update attributes. My point was
> only for the third argument of setup_subscriber().
>
Fixed.
> 02. setup_subscriber()
> ```
> + pg_log_info("cleaning up existing publications on the subscriber");
> ```
>
> I don't think this output is needed. check_and_drop_existing_publications() has the similar output.
>
Fixed.
> 03. drop_publication_by_name()
> ```
> +
> + appendPQExpBuffer(query, "DROP PUBLICATION %s", pubname_esc);
> + pg_log_info("dropping publication %s in database \"%s\"", pubname, dbname);
> + pg_log_debug("command is: %s", query->data);
>
> ```
>
> Currently, appendPQExpBuffer() is done after the pg_log_info(). Please reserve current ordering if
> there are no reasons. Also, variable "str" is currently used instead of query, please follow.
>
Fixed.
> 04. drop_publication_by_name()
> ```
> if (!dry_run)
> {
> - res = PQexec(conn, str->data);
> + res = PQexec(conn, query->data);
> if (PQresultStatus(res) != PGRES_COMMAND_OK)
> + dbinfo->made_publication = false;
> + else
> {
> - 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. */
> -
> - /*
> - * Don't disconnect and exit here. This routine is used by primary
> - * (cleanup publication / replication slot due to an error) and
> - * subscriber (remove the replicated publications). In both cases,
> - * it can continue and provide instructions for the user to remove
> - * it later if cleanup fails.
> - */
> + pg_log_error("could not drop publication %s in database \"%s\": %s",
> + pubname, dbname, PQresultErrorMessage(res));
> }
> ```
>
> pg_log_error() is exected when the command succeed: This is not correct.
>
Fixed.
> 05. 040_pg_createsubscriber.pl
> ```
> +# 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;
> ```
>
> I don't think new primary is not needed. Can't you reuse node_p?
>
Fixed.
The attached patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
Attachment
Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
Shubham Khanna
Date:
On Tue, Feb 25, 2025 at 4:50 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shubham,
>
> Thanks for updating the patch. Few comments.
>
> 01. CreateSubscriberOptions
> ```
> + bool cleanup_existing_publications; /* drop all publications */
> ```
>
> My previous comment [1] #1 did not intend to update attributes. My point was
> only for the third argument of setup_subscriber().
>
> 02. setup_subscriber()
> ```
> + pg_log_info("cleaning up existing publications on the subscriber");
> ```
>
> I don't think this output is needed. check_and_drop_existing_publications() has the similar output.
>
> 03. drop_publication_by_name()
> ```
> +
> + appendPQExpBuffer(query, "DROP PUBLICATION %s", pubname_esc);
> + pg_log_info("dropping publication %s in database \"%s\"", pubname, dbname);
> + pg_log_debug("command is: %s", query->data);
>
> ```
>
> Currently, appendPQExpBuffer() is done after the pg_log_info(). Please reserve current ordering if
> there are no reasons. Also, variable "str" is currently used instead of query, please follow.
>
> 04. drop_publication_by_name()
> ```
> if (!dry_run)
> {
> - res = PQexec(conn, str->data);
> + res = PQexec(conn, query->data);
> if (PQresultStatus(res) != PGRES_COMMAND_OK)
> + dbinfo->made_publication = false;
> + else
> {
> - 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. */
> -
> - /*
> - * Don't disconnect and exit here. This routine is used by primary
> - * (cleanup publication / replication slot due to an error) and
> - * subscriber (remove the replicated publications). In both cases,
> - * it can continue and provide instructions for the user to remove
> - * it later if cleanup fails.
> - */
> + pg_log_error("could not drop publication %s in database \"%s\": %s",
> + pubname, dbname, PQresultErrorMessage(res));
> }
> ```
>
> pg_log_error() is exected when the command succeed: This is not correct.
>
> 05. 040_pg_createsubscriber.pl
> ```
> +# 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;
> ```
>
> I don't think new primary is not needed. Can't you reuse node_p?
>
> [1]:
https://www.postgresql.org/message-id/OSCPR01MB14966D845AC77FC46ECEECCE4F5C72%40OSCPR01MB14966.jpnprd01.prod.outlook.com
>
The v10 patch at [1] required rebasing on the latest HEAD. I have
prepared a rebased version and updated the patch accordingly.
[1] - https://www.postgresql.org/message-id/CAHv8RjJtzXc4BWoKTyTB-9WEcAJ4N5qsD6YuXK3EAmsT6237yw%40mail.gmail.com
The attached patch includes the suggested changes.
Thanks and regards,
Shubham Khanna.
Attachment
RE: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
"Hayato Kuroda (Fujitsu)"
Date:
Dear Shubham, I did a quick check and have a comment in drop_publication(). ``` + /* For cleanup purposes */ + dbinfos.dbinfo->made_publication = true; + ``` Please do not modify randomly. Currently, made_publication is set to false when the command fails. No need to set to true here - Please follow that. Also, please do not remove comments within the if statement. Best regards, Hayato Kuroda FUJITSU LIMITED
Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
Shubham Khanna
Date:
On Mon, Mar 3, 2025 at 5:59 PM Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> wrote: > > Dear Shubham, > > I did a quick check and have a comment in drop_publication(). > > ``` > + /* For cleanup purposes */ > + dbinfos.dbinfo->made_publication = true; > + > ``` > > Please do not modify randomly. Currently, made_publication is set to false when > the command fails. No need to set to true here - Please follow that. Also, please > do not remove comments within the if statement. > Fixed the suggested changes. The attached patch contains the required changes. Thanks and regards, Shubham Khanna.
Attachment
Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
From
Nisha Moond
Date:
On Tue, Mar 4, 2025 at 10:30 AM Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> Fixed the suggested changes. The attached patch contains the required changes.
>
Hi Shubham,
Thanks for the patch! I tested its functionality and didn't find any
issues so far. I'll continue with further testing.
Here are some review comments for v12 patch:
src/bin/pg_basebackup/pg_createsubscriber.c
1)
+ printf(_(" -c --cleanup-existing-publications\n"
+ " drop all publications on the
subscriber\n"));
Similar to docs, here too, we should clarify that publications will be
dropped only from the specified databases, not all databases.
Suggestion -
"drop all publications from specified databases on the subscriber\n"
2)
@@ -1171,10 +1179,12 @@ check_and_drop_existing_subscriptions(PGconn *conn,
/*
* 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' is true, existing publications on
+ * the subscriber will be dropped before creating new subscriptions.
*/
static void
-setup_subscriber(struct LogicalRepInfo *dbinfo, const char *consistent_lsn)
+setup_subscriber(struct LogicalRepInfo *dbinfo, const char *consistent_lsn,
+ bool cleanup_existing_publications)
{
It is not clear that the 'drop_publications' value comes from the
command. How about replacing it with:
/If 'drop_publications' is/If 'drop_publications' option is/
OR
If you meant to refer to the specific parameter
'cleanup_existing_publications', please use the exact name.
3)
@@ -1195,8 +1205,13 @@ setup_subscriber(struct LogicalRepInfo *dbinfo,
const char *consistent_lsn)
* Since the publication was created before the consistent LSN, it is
* available on the subscriber when the physical replica is promoted.
* Remove publications from the subscriber because it has no use.
+ * Additionally, drop publications existed before this command if
+ * requested.
*/
- drop_publication(conn, &dbinfo[i]);
+ if (cleanup_existing_publications)
+ check_and_drop_existing_publications(conn, dbinfo[i].dbname);
+ else
+ drop_publication_by_name(conn, dbinfo[i].pubname, dbinfo[i].dbname);
The existing comment only refers to removing the new publication
created for the current process and does not mention existing ones.
With this patch changes, it is unclear which publications are being
referenced when saying "Remove publications ..."(plural), and the
phrase "before this command" is ambiguous—it's not clear which command
is being referred to. The comment should be reworded for better
clarity.
Suggestion -
"Since the publication was created before the consistent LSN, it
remains on the subscriber even after the physical replica is
promoted. Remove this publication from the subscriber because
it has no use. Additionally, if requested, drop all pre-existing
publications."
4)
+/* Drop a single publication, given in dbinfo. */
+static void
+drop_publication_by_name(PGconn *conn, const char *pubname, const char *dbname)
+{
Since "dbinfo" is no longer passed to this function, the function
comments should be updated accordingly. Also, use the same format as
other function comments.
Suggestion-
/*
* Drop the specified publication of the given database.
*/
5)
+ pg_log_info("dropping publication %s in database \"%s\"", pubname, dbname);
+
Publication names should be enclosed in double quotes ("") in logs, as
previously done.
6)
+ pg_log_error("could not drop publication %s in database \"%s\": %s",
+ pubname, dbname, PQresultErrorMessage(res));
Same as #5, use double quotes for the publication name.
--
Thanks,
Nisha