MIME-Version: 1.0
References: <CAHv8RjL4OvoYafofTb_U_JD5HuyoNowBoGpMfnEbhDSENA74Kg@mail.gmail.com>
 <OSCPR01MB1496664FDC38DA40A441F449FF5EE2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
 <CAHv8RjJw2xDyWgagdAL7RBUEEEZtKcVJO_gNjQQHkUj8QrmwuQ@mail.gmail.com>
 <CANhcyEUkQybOe=J5KypWQ80deULBLjjsdyP+DF57+BwSni57Vw@mail.gmail.com>
 <CAHv8Rj+1v6257anqijp90f3cdZvSOhkm_ZRQRVU1C_0OYq056g@mail.gmail.com>
 <OSCPR01MB1496679AD9C3C47DB30542ECBF5F12@OSCPR01MB14966.jpnprd01.prod.outlook.com>
 <CAHv8RjJGjA_fwJ_TNrHbyTz7ATA1MXfNxPhoBvJfKRfk1Qf-ng@mail.gmail.com>
 <CANhcyEXfn-TGW2L991vXAaGH-UVBCAZuDUSRFTyVTQPbDgamdA@mail.gmail.com> <CAHv8RjKGywu+3cAv9MPgxi1_TUXBT8yzUsW+f=g5UsgeJ8Uk6g@mail.gmail.com>
In-Reply-To: <CAHv8RjKGywu+3cAv9MPgxi1_TUXBT8yzUsW+f=g5UsgeJ8Uk6g@mail.gmail.com>
From: Shlok Kyal <shlok.kyal.oss@gmail.com>
Date: Thu, 13 Feb 2025 17:35:56 +0530
Message-ID: <CANhcyEUo7F954LULk859xs6FtwQ5USH6C2tiBbGwpihU2yHmAQ@mail.gmail.com>
Subject: Re: Adding a '--clean-publisher-objects' option to
 'pg_createsubscriber' utility.
To: Shubham Khanna <khannashubham1197@gmail.com>
Cc: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CANhcyEUo7F954LULk859xs6FtwQ5USH6C2tiBbGwpihU2yHmAQ%40mail.gmail.com>
Precedence: bulk

On Thu, 13 Feb 2025 at 15:20, Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> On Tue, Feb 11, 2025 at 9:56=E2=80=AFPM 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=E2=80=AFAM 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_ex=
isting_subscriptions().
> > > > As for the replication slot, I have told in [2], it would be create=
d 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-publica=
tions"
> > >
> > > > 02. usage()
> > > > ```
> > > > +       printf(_("  -C  --cleanup-publisher-objects drop all public=
ations on the logical replica\n"));
> > > > ```
> > >
> > > Fixed.
> > >
> > > > s/logical replica/subscriber
> > > >
> > > > 03. drop_all_publications
> > > > ```
> > > > +/* Drops all existing logical replication publications from all su=
bscriber
> > > > + * 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 enoug=
h.
> > > >
> > >
> > > 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() ther=
e 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 TABL=
ES;");
> > > > +$node_p->safe_psql($db1, "CREATE PUBLICATION test_pub2 FOR ALL TAB=
LES;");
> > > > +
> > > > +# Verify the existing publications
> > > > +my $pub_count_before =3D
> > > > +  $node_s->safe_psql($db1, "SELECT COUNT(*) FROM pg_publication;")=
;
> > > > +is($pub_count_before, '2',
> > > > +       'two publications created before --cleanup-publisher-object=
s 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 TABL=
ES;");
> > > > +$node_p->safe_psql($db1, "CREATE PUBLICATION test_pub2 FOR ALL TAB=
LES;");
> > > > +
> > > > +# Verify the existing publications
> > > > +my $pub_count_before =3D
> > > > +  $node_s->safe_psql($db1, "SELECT COUNT(*) FROM pg_publication;")=
;
> > > > +is($pub_count_before, '2',
> > > > +       'two publications created before --cleanup-publisher-object=
s is run');
> > > > +
> > > > ```
> > > >
> > > > Also, there is a possibility that CREATE PUBLICATION on node_p is n=
ot replicated yet
> > > > when SELECT COUNT(*) is executed. Please wait for the replay.
> > > >
> > > > [1]: https://www.postgresql.org/message-id/CAHv8RjL4OvoYafofTb_U_JD=
5HuyoNowBoGpMfnEbhDSENA74Kg%40mail.gmail.com
> > > > [2]: https://www.postgresql.org/message-id/OSCPR01MB1496664FDC38DA4=
0A441F449FF5EE2%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 ser=
ver.

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=3DDBNAME           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(*) =3D 2 FROM pg_publication"),
+ 'two publications created before --cleanup-existing-publications is run')=
;
+

Should we use 'safe_psql'?

Thanks and Regards,
Shlok Kyal