On Fri, Feb 7, 2025 at 7:46 AM Hayato Kuroda (Fujitsu) <kuroda.hay...@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.
Thanks and regards,
Shubham Khanna.
v4-0001-Support-for-dropping-all-publications-in-pg_creat.patch
Description: Binary data
