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

On Tue, 11 Feb 2025 at 09:51, Shubham Khanna
<khannashubham1197(at)gmail(dot)com> wrote:
>
> On Fri, Feb 7, 2025 at 7:46 AM Hayato Kuroda (Fujitsu)
> <kuroda(dot)hayato(at)fujitsu(dot)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