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

On Fri, Feb 14, 2025 at 4:57 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> On Fri, 14 Feb 2025 at 10:50, Shubham Khanna
> <khannashubham1197(at)gmail(dot)com> wrote:
> >
> > On Thu, Feb 13, 2025 at 5:36 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> > >
> > > On Thu, 13 Feb 2025 at 15:20, Shubham Khanna
> > > <khannashubham1197(at)gmail(dot)com> wrote:
> > > >
> > > > On Tue, Feb 11, 2025 at 9:56 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> > > > >
> > > > > 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.
> > > > > */
> > > > >
> > > >
> > > > 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
>

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

Thanks and regards,
Shubham Khanna.