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

From: Shlok Kyal <[email-masked]>
Date: Thu, 13 Feb 2025 17:35:56 +0530

On Thu, 13 Feb 2025 at 15:20, Shubham Khanna
<[email-masked]> wrote:
>
> On Tue, Feb 11, 2025 at 9:56 PM Shlok Kyal <[email-masked]> wrote:
> >
> > On Tue, 11 Feb 2025 at 09:51, Shubham Khanna
> > <[email-masked]> wrote:
> > >
> > > On Fri, Feb 7, 2025 at 7:46 AM Hayato Kuroda (Fujitsu)
> > > <[email-masked]> 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

---

From: Shubham Khanna <[email-masked]>
Date: Fri, 14 Feb 2025 10:50:41 +0530

> Should we use 'safe_psql'?

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

Thanks and regards,
Shubham Khanna.

---

From: Shlok Kyal <[email-masked]>
Date: Fri, 14 Feb 2025 16:57:01 +0530

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

---

From: Shubham Khanna <[email-masked]>
Date: Fri, 14 Feb 2025 17:23:08 +0530

Thanks and regards,
Shubham Khanna.

---

From: Peter Smith <[email-masked]>
Date: Mon, 17 Feb 2025 15:18:52 +1100

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.
+

/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

---

From: vignesh C <[email-masked]>
Date: Tue, 18 Feb 2025 10:18:07 +0530

> 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

---

From: Shubham Khanna <[email-masked]>
Date: Thu, 20 Feb 2025 12:14:27 +0530

> 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.

> comment message didn't describe this clearly.

Modified the commit message.

Fixed.

Fixed.

Fixed.

Fixed.

Fixed.

> 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.

Thanks and regards,
Shubham Khanna.

---

From: Peter Smith <[email-masked]>
Date: Fri, 21 Feb 2025 14:00:40 +1100

> On Mon, Feb 17, 2025 at 9:49 AM Peter Smith <[email-masked]> wrote:
...
> will revise it in the next patch version once there is a consensus on

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?

======

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.

======

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

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Fri, 21 Feb 2025 09:36:35 +0000

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);
		}

		/*
		 * Additionally, drop publications existed before this command if
		 * requested.

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

---

From: Shubham Khanna <[email-masked]>
Date: Tue, 25 Feb 2025 14:21:28 +0530

Fixed.

Fixed.

Fixed.

> 'check_and_drop_existing_subscriptions()

Added a new function 'check_and_drop_exixsting_publications()'

Fixed.

Removed this function.

Fixed.

> The 'count' variable seems unused.

Removed the usage of 'count'.

Fixed.

Fixed.

Fixed.

Fixed.

> 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.

Thanks and regards,
Shubham Khanna.

---

From: Shubham Khanna <[email-masked]>
Date: Tue, 25 Feb 2025 14:24:39 +0530

Fixed.

Fixed.

Fixed.

Removed this function.

Fixed.

Fixed.

Fixed.

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.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 25 Feb 2025 11:20:28 +0000

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

---

From: Shubham Khanna <[email-masked]>
Date: Tue, 25 Feb 2025 22:19:06 +0530

Fixed.

Fixed.

Fixed.

Fixed.

Fixed.

Thanks and regards,
Shubham Khanna.

---

From: Shubham Khanna <[email-masked]>
Date: Fri, 28 Feb 2025 15:59:49 +0530

> [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.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Mon, 3 Mar 2025 12:29:15 +0000

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

---

From: Shubham Khanna <[email-masked]>
Date: Tue, 4 Mar 2025 10:29:39 +0530

> do not remove comments within the if statement.

Fixed the suggested changes. The attached patch contains the required changes.

Thanks and regards,
Shubham Khanna.

---

From: Nisha Moond <[email-masked]>
Date: Tue, 4 Mar 2025 16:30:50 +0530

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:

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)

Publication names should be enclosed in double quotes ("") in logs, as
previously done.

6)

Same as #5, use double quotes for the publication name.

--
Thanks,
Nisha

---

From: Shubham Khanna <[email-masked]>
Date: Tue, 4 Mar 2025 20:05:31 +0530

> "drop all publications from specified databases on the subscriber\n"

The suggested message was exceeding 95 words, so I have modified it to:-
"drop all publications from specified subscriber databases\n"

Fixed.

Fixed.

Fixed.

Fixed.

Fixed.

Thanks and regards,
Shubham Khanna.

---

From: Nisha Moond <[email-masked]>
Date: Wed, 5 Mar 2025 15:54:48 +0530

Hi Shubham,

Here are few comments for 040_pg_createsubscriber.pl

1)
+# Run pg_createsubscriber on node S using '--cleanup-existing-publications'.
+# --verbose is used twice to show more information.

  1a) /node S/ node S1

  1b) Also, can we keep the comment in the same pattern as it was earlier -
# Run pg_createsubscriber on node S1. --verbose is used twice
# to show more information.
# In passing, also test the --enable-two-phase and
--cleanup-existing-publications
# options.

2)
+# Reuse P as primary
+# Set up node S2 as standby linking to node P
+$node_p->backup('backup_3');

/Reuse P as/ Reuse node P as/

3)
+$node_s2->append_conf(
+ 'postgresql.conf', qq[
+       primary_conninfo = '$pconnstr'
+       hot_standby_feedback = on
+       max_logical_replication_workers = 5
+       ]);

Do we need "hot_standby_feedback = on" on node_s2? I think we can remove it.

4)
+# Create user-defined publications
+$node_p->safe_psql($db3, "CREATE PUBLICATION test_pub3 FOR ALL TABLES;");
+$node_p->safe_psql($db3, "CREATE PUBLICATION test_pub4 FOR ALL TABLES;");

Can create both publications under one safe_psql as -

$node_p->safe_psql($db3, qq[CREATE PUBLICATION test_pub3 FOR ALL TABLES;
    CREATE PUBLICATION test_pub4 FOR ALL TABLES;
]);

5)
+# Run pg_createsubscriber on node A without using
+# '--cleanup-existing-publications'.
+# --verbose is used twice to show more information.

  5a)  /node A/node S2/
  5b)  /without using '--cleanup-existing-publications' / without
'--cleanup-existing-publications' option

6)
+ ],
+ 'run pg_createsubscriber without --cleanup-existing-publications on node A'
+);

/node A/node S2/

7)
+# Drop the newly created publications
+$node_p->safe_psql($db3, "DROP PUBLICATION IF EXISTS test_pub3;");
+$node_p->safe_psql($db3, "DROP PUBLICATION IF EXISTS test_pub4;");

Similar to #4, use single safe_psql to drop both the publications.
OTOH, do we really need to drop the publications here? I think we can
remove this part since we're performing cleanup right after.
~~~~

--
Thanks,
Nisha

---

From: Peter Smith <[email-masked]>
Date: Thu, 6 Mar 2025 14:56:33 +1100

Hi Shubham.

Some review comments for patch v13-0001.

======
GENERAL

1.

I've never liked this proposed switch name much.

e.g. why say "cleanup" instead of "drop"? What is the difference?
Saying drop is very explicit about what the switch will do.
e.g. why say "existing"? It seems redundant; you can't cleanup/drop
something that does not exist.

My preference is one of:
--drop-publications
--drop-all-publications

either of which seem nicely aligned with the descriptions in the usage and docs.

Yeah, I know I have raised this same point before, but AFAICT the
reply was like "will revise it in the next patch version", but that
was many versions ago. I think it is important to settle the switch
name earlier than later because there will be many tentacles into the
code (vars, params, fields, comments) and docs if anything changes --
so it is not a decision you want to leave till the end because it
could destabilise everything at the last minute.

======
Commit message

2.
By default, publications are preserved to avoid unintended data loss.

~

Was there supposed to be a blank line before this text, or should this
text be wrapped into the preceding paragraph?

======

setup_subscriber:

3.
 /*
  * 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' options is true, existing
+ * publications on the subscriber will be dropped before creating new
+ * subscriptions.
  */

There are multiple things amiss with this comment.
- 'drop_publications' is not the parameter name
- 'drop_publications' options [sic plural??]. It is not an option
here; it is a parameter

~~~

check_and_drop_existing_publications:

4.
 /*
- * Remove publication if it couldn't finish all steps.
+ * Check and drop existing publications on the subscriber if requested.
  */

There is no need to say "if requested.". It is akin to saying this
function does XXX if this function is called.

~~~

drop_publication_by_name:

5.
+/* Drop the specified publication of the given database. */
+static void
+drop_publication_by_name(PGconn *conn, const char *pubname, const char *dbname)

5a.
I think it is better to define this function before
check_and_drop_existing_publications. YMMV.

~

5b.
IMO the parameters should be reordered (PGconn *conn, const char
*dbname, const char *pubname). It seems more natural and would be
consistent with check_and_drop_existing_publications. YMMV.

~~~

6.
- dbinfo->pubname, dbinfo->dbname, PQresultErrorMessage(res));
- dbinfo->made_publication = false; /* don't try again. */
+ pubname, dbname, PQresultErrorMessage(res));
+ dbinfos.dbinfo->made_publication = false; /* don't try again. */

Something about this flag assignment seems odd to me. IIUC
'made_publications' is for removing the cleanup_objects_atexit to be
able to remove the special publication implicitly made by the
pg_createsubscriber. But, AFAIK we can also get to this code via the
--cleanup-existing-publication switch, so actually it might be one of
the "existing" publication DROPS that has failed. If so, then the
"don't try again comment" is misleading because it may not be that
same publication "again" at all.

======

GENERAL.

7.
Most of the changes to this test code are just changing node name S ->
S1. It's not much to do with the patch other than tidying it in
preparation for a new node S2.  OTOH it makes the review far harder
because there are so many changes. IMO all this S->S1 stuff should be
done as a separate pre-requisite patch and then it will be far easier
to see what changes are added for the --clean-existing-publications
testing.

~~~

8.
 # Set up node S as standby linking to node P
 $node_p->backup('backup_1');
-my $node_s = PostgreSQL::Test::Cluster->new('node_s');
-$node_s->init_from_backup($node_p, 'backup_1', has_streaming => 1);
-$node_s->append_conf(
+my $node_s1 = PostgreSQL::Test::Cluster->new('node_s1');
+$node_s1->init_from_backup($node_p, 'backup_1', has_streaming => 1);
+$node_s1->append_conf(

The comment should refer to S1, not S.

~~~

9.
 # Set up node C as standby linking to node S
-$node_s->backup('backup_2');
+$node_s1->backup('backup_2');
 my $node_c = PostgreSQL::Test::Cluster->new('node_c');
-$node_c->init_from_backup($node_s, 'backup_2', has_streaming => 1);
+$node_c->init_from_backup($node_s1, 'backup_2', has_streaming => 1);

The comment should refer to S1, not S.

~~~

10.
 # Check some unmet conditions on node S
-$node_s->append_conf(
+$node_s1->append_conf(

The comment should refer to S1, not S.

(note... there are lots of these. You should search/fix them all;
these review comments might miss some)

~~~

11.
+ '--socketdir' => $node_s1->host,
+ '--subscriber-port' => $node_s1->port,
  '--database' => $db1,
  '--database' => $db2,
  ],
  'standby contains unmet conditions on node S');

The message should refer to S1, not S.

(note... there are lots of these. You should search/fix them all;
these review comments might miss some)

~~~

12.
 # dry run mode on node S
 command_ok(
@@ -338,10 +353,10 @@ command_ok(
  '--verbose',
  '--dry-run',
  '--recovery-timeout' => $PostgreSQL::Test::Utils::timeout_default,
- '--pgdata' => $node_s->data_dir,
+ '--pgdata' => $node_s1->data_dir,
  '--publisher-server' => $node_p->connstr($db1),
- '--socketdir' => $node_s->host,
- '--subscriber-port' => $node_s->port,
+ '--socketdir' => $node_s1->host,
+ '--subscriber-port' => $node_s1->port,
  '--publication' => 'pub1',
  '--publication' => 'pub2',
  '--subscription' => 'sub1',
@@ -352,10 +367,11 @@ command_ok(
  'run pg_createsubscriber --dry-run on node S');
Comment and Message should refer to S1, not S.

~~~

13.
 # Check if node S is still a standby
-$node_s->start;
-is($node_s->safe_psql('postgres', 'SELECT pg_catalog.pg_is_in_recovery()'),
- 't', 'standby is in recovery');
-$node_s->stop;
+$node_s1->start;
+is( $node_s1->safe_psql('postgres', 'SELECT pg_catalog.pg_is_in_recovery()'),
+ 't',
+ 'standby is in recovery');
+$node_s1->stop;

The comment should refer to S1, not S.

~~~

14.
-# Run pg_createsubscriber on node S.  --verbose is used twice
-# to show more information.
+# Run pg_createsubscriber on node S using '--cleanup-existing-publications'.
+# --verbose is used twice to show more information.
 # In passing, also test the --enable-two-phase option
 command_ok(
  [
  'pg_createsubscriber',
  '--verbose', '--verbose',
  '--recovery-timeout' => $PostgreSQL::Test::Utils::timeout_default,
- '--pgdata' => $node_s->data_dir,
+ '--pgdata' => $node_s1->data_dir,
  '--publisher-server' => $node_p->connstr($db1),
- '--socketdir' => $node_s->host,
- '--subscriber-port' => $node_s->port,
+ '--socketdir' => $node_s1->host,
+ '--subscriber-port' => $node_s1->port,
  '--publication' => 'pub1',
  '--publication' => 'pub2',
  '--replication-slot' => 'replslot1',
  '--replication-slot' => 'replslot2',
  '--database' => $db1,
  '--database' => $db2,
- '--enable-two-phase'
+ '--enable-two-phase',
+ '--cleanup-existing-publications',
  ],
  'run pg_createsubscriber on node S');

Comment and Message should refer to S1, not S.

~~~

15.

Probably these can be combined.

~~~

16.

Probably these can be combined.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Shubham Khanna <[email-masked]>
Date: Mon, 10 Mar 2025 12:00:50 +0530

Fixed.

Fixed.

> Do we need "hot_standby_feedback = on" on node_s2? I think we can remove it.

Removed and updated the configurations.

Fixed.

Fixed.

Fixed.

Fixed.

Thanks and regards,
Shubham Khanna.

---

From: Shubham Khanna <[email-masked]>
Date: Mon, 10 Mar 2025 12:11:06 +0530

> could destabilise everything at the last minute.

I have updated the option name to '--drop-all-publications'.

Fixed.

Fixed.

Fixed.

Fixed.

> same publication "again" at all.

I agree with your point. The current comment is misleading, as the
failure could be related to an existing publication drop via
--cleanup-existing-publications now --drop-all-publications, not just
the publication created by pg_createsubscriber.
This issue is still open, and I will address it in the next version of
the patch.

> to see what changes are added for the --clean-existing-publications

I understand your concern. Since I am using two nodes (node_s1 and
node_s2), I will work on creating a separate prerequisite patch for
renaming S -> S1. This should make it easier to review the actual
changes related to --cleanup-existing-publications now
--drop-all-publications testing.

Fixed.

Fixed.

Fixed.

Fixed.

Fixed.

Fixed.

Fixed.

Fixed.

Fixed.

The attached patch at [1] contains the required changes.

[1] - https://www.postgresql.org/message-id/CAHv8RjLCZyzFMGR8SBdvSocRZGAAr_eRd4ht48kXCp9oEaKQeQ%40mail.gmail.com

Thanks and regards,
Shubham Khanna.

---

From: Nisha Moond <[email-masked]>
Date: Mon, 10 Mar 2025 15:09:01 +0530

> This issue is still open, and I will address it in the next version of

1) We should set "made_publication = false" only if the failure occurs
for the special publication implicitly created by pg_createsubscriber.
If the error is in any other publication, this special publication
should still be cleaned up by the cleanup_objects_atexit.

2) This part of code has another bug:
 "dbinfos.dbinfo->made_publication = false;" incorrectly modifies
dbinfo[0], even if the failure occurs in other databases (dbinfo[1],
etc.). Since cleanup_objects_atexit() checks made_publication per
database, this could lead to incorrect behavior.
Solution: Pass the full 'dbinfo' structure to
drop_publication_by_name() , and use it accordingly.

--
Thanks,
Nisha

---

From: Shubham Khanna <[email-masked]>
Date: Mon, 10 Mar 2025 16:31:02 +0530

Fixed.

Fixed.

Thanks and regards,
Shubham Khanna.

---

From: Nisha Moond <[email-masked]>
Date: Mon, 10 Mar 2025 17:01:55 +0530

> The attached patch contains the suggested changes.

Thanks for the patch. Here are few comments for the new change -
1)
+static void check_and_drop_existing_publications(PGconn *conn, struct
LogicalRepInfo *dbinfo);
 - move the second parameter to the next line to maintain 80 char length.

2)
  pg_log_error("could not drop publication \"%s\" in database \"%s\": %s",
- dbinfo->pubname, dbinfo->dbname, PQresultErrorMessage(res));
+ pubname, dbinfo->dbname,
+ PQresultErrorMessage(res));

You can keep "PQresultErrorMessage(res));" in the previous line, it
will still be < 80 chars.

3) The new IF check -
+           if (strcmp(pubname, dbinfos.dbinfo->pubname) == 0)
+               dbinfo->made_publication = false;   /* don't try again. */

always compares dbinfo[0]->pubname, but it should compare 'pubname'
with the respective database's publication. Please correct it as -
        if (strcmp(pubname, dbinfo->pubname) == 0)

--
Thanks,
Nisha

---

From: Shubham Khanna <[email-masked]>
Date: Mon, 10 Mar 2025 20:12:35 +0530

Fixed.

Fixed.

Fixed.

Thanks and regards,
Shubham Khanna.

---

From: Peter Smith <[email-masked]>
Date: Tue, 11 Mar 2025 11:00:16 +1100

Hi Shubham.

Some review comments for patch v16-0001.

======

1.
+     <term><option>-c</option></term>
+     <term><option>--drop-all-publications</option></term>

Is 'c' the best switch choice letter for this option? It doesn't seem
intuitive, but unfortunately, I don't have any better ideas since d/D
and p/P are already being used.

======

CreateSubscriberOptions:

2.
  SimpleStringList sub_names; /* list of subscription names */
  SimpleStringList replslot_names; /* list of replication slot names */
  int recovery_timeout; /* stop recovery after this time */
+ bool drop_publications; /* drop all publications */

Now that you have modified everywhere to rename the parameter as
'drop_all_publications', I think you should change this field name too
so everything is consistent.

~~~

3.
 static void create_publication(PGconn *conn, struct LogicalRepInfo *dbinfo);
-static void drop_publication(PGconn *conn, struct LogicalRepInfo *dbinfo);
+static void drop_publication_by_name(PGconn *conn, const char *pubname,
+ struct LogicalRepInfo *dbinfo);
+static void check_and_drop_existing_publications(PGconn *conn,
+ struct LogicalRepInfo *dbinfo);

I think the parameters of drop_publication_by_name should be reordered
like "(PGconn *conn, struct LogicalRepInfo *dbinfo, const char
*pubname)", to make the 1st 2 params consistent with other functions.
I wrote this already in my previous review. You replied "Fixed"
[1-#5b], but it isn't.

~~~

setup_subscriber:

4.
 /*
  * 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' parameter is true, existing
+ * publications on the subscriber will be dropped before creating new
+ * subscriptions.
  */

The comment refers to 'drop_publications', but the parameter name is
'drop_all_publications'

~~~

5.
  if (PQresultStatus(res) != PGRES_COMMAND_OK)
  {
  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. */
+ pubname, dbinfo->dbname, PQresultErrorMessage(res));
+
+ if (strcmp(pubname, dbinfo->pubname) == 0)
+ dbinfo->made_publication = false; /* don't try again. */

5a.
IMO this strcmp code needs a comment to say what this logic is doing.
e.g. IIUC dbinfo->pubname is the name of the internal FOR ALL TABLES
publication that the tool had created.

~

5b.
This logic seems flawed to me. Maybe I am mistaken, but AFAIK when you
say '--drop-all-publication' that is going to drop all the existing
publications but it is *also* going to drop the internally created FOR
ALL TABLES publication. Therefore, to prevent the exit handler from
attempting to double-delete the same internal publication don't you
need to set dbinfo->made_publication = false also in the case of
*successful* drop of dbinfo->pubname?

~

5c. Consider maybe also checking if made_publication is true, to avoid
making unnecessary calls to strcmp e.g.
if (dbinfo->made_publication && strcmp(pubname, dbinfo->pubname) == 0)

======

6.
The test code remains difficult to review because I can't see the
forest for the trees due to the dozens of S->S1 node name changes.
These name changes are unrelated to the new feature so please separate
them into a different prerequisite patch so we can just focus on what
changes were made just for --drop-all-publications. I know you already
said you are working on it [1-#7], but multiple patch versions have
been posted since you said that.

======
[1] v13 review https://www.postgresql.org/message-id/CAHv8RjKmf5bAcgTmGToWSn_Fx%2BO2y8qCiLScBXvBTD0D5gX2sw%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: "David G. Johnston" <[email-masked]>
Date: Mon, 10 Mar 2025 17:31:41 -0700

> and p/P are already being used.

Agreed.  Better to just not assign a short name to this.

The description for the sgml docs needs to frame up this option's purpose.

How exactly does one go about backing up a publication?  You discuss the
topic in the commit message but that definitely seems user-facing.

If we aren't expecting lots of publications maybe name them individually
instead of logging "all publications"?  Possibly one info line each but
even just comma separated would work.

The name of this is shock-inducing.  Admittedly, after pondering things, it
is fairly obvious that only the target is going to be affected, but there
is a source database involved here as well.  It is also unclear on whether
it would happen before or after, which is less problematic since it would
only impact failure modes anyway - when all is said and done with this
specified upon restart following the pg_resetwal the server will have no
publications, right?

Maybe: --drop-target-publications-first ?

If we do want a letter either "X" or "Z" probably works for an
English-speaking audience probably.  X is how one denotes removing
something; and Z is a mnemonic for "Zap" which is a synonym for "Drop". "R"
for "Remove".

Can you briefly recap how this is different than the automatic behavior
described in the existing Step 6?
"Drop publications on the target server that were replicated because they
were created before the replication start location. It has no use on the
subscriber."

-R --remove-target-publications; I fine with answering the timing question
to the description.

David J.

---

From: Peter Smith <[email-masked]>
Date: Tue, 11 Mar 2025 18:20:04 +1100

On Tue, Mar 11, 2025 at 11:32 AM David G. Johnston
<[email-masked]> wrote:
> How exactly does one go about backing up a publication?  You discuss the topic in the commit message but that definitely seems user-facing.
>
> If we aren't expecting lots of publications maybe name them individually instead of logging "all publications"?  Possibly one info line each but even just comma separated would work.
>
> The name of this is shock-inducing.  Admittedly, after pondering things, it is fairly obvious that only the target is going to be affected, but there is a source database involved here as well.  It is also unclear on whether it would happen before or after, which is less problematic since it would only impact failure modes anyway - when all is said and done with this specified upon restart following the pg_resetwal the server will have no publications, right?
>
> If we do want a letter either "X" or "Z" probably works for an English-speaking audience probably.  X is how one denotes removing something; and Z is a mnemonic for "Zap" which is a synonym for "Drop". "R" for "Remove".
>
> Can you briefly recap how this is different than the automatic behavior described in the existing Step 6?
> "Drop publications on the target server that were replicated because they were created before the replication start location. It has no use on the subscriber."
>
> -R --remove-target-publications; I fine with answering the timing question to the description.
>

Hi David.

This feature is all about removing some objects that were replicated
during the streaming replication before pg_createsubscriber was run,
but that are no longer needed/wanted afterwards.

Although the current thread/patch is just for removing existing
publications from the target server, in the future there might be some
enhancements to remove other kinds of unwanted objects that were
replicated to the target server by streaming replication -- e.g.
remove subscriptions, or remove replication slots, or whatever.

Unfortunately, we are spinning in circles a bit trying to come up with
a good way to represent the option needed for this, while at the same
time trying to be future-proof. I see 3 choices...

======

Choice 1.  Generic option

Implement a single boolean option to remove everything.

In this case, the option would need to have some generic name like
"--remove-target-existing-object". For now (current patch), it would
be implemented to remove only existing publications. In the future, it
could remove more things. But, there isn't much flexibility: IIUC this
option can only remove either all or none.

~

Choice 2. Different options for removing different things (the current
patch is like this)

"--remove-target-publications" is what this patch is doing.

In future, if we want to remove more things then we would need to add
more options like "--remove-target-subscriptions",
"--remove-target-replication-slots" etc.

~

Choice 3. Implement some option that has an argument saying what to delete

Implement an option that takes some argument saying what objects to remove.

Here, the current patch would be something like:
"--remove-target-objects=publications". In future, this option could
be enhanced to accept other values like
"--remove-target-objects=publications,subscriptions" etc.

~~~

Do you have any thoughts on what kind of option is best here?

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: "David G. Johnston" <[email-masked]>
Date: Tue, 11 Mar 2025 07:50:57 -0700

> Do you have any thoughts on what kind of option is best here?
Option 1 by far.  Though personally I'd rather do something like what
pg_upgrade does and output a script with drop commands that can be executed
via psql instead of having pg_createsubscriber execute said commands.  I'd
call it "pruning the target".

Any automated bash script I'd write would just do:

pg_createsubscriber ... --prune-target-script=prune.sql
psql --file prune.sql

And if I'm working interactively I'd want to spend the extra minute or so
reviewing the commands in prune.sql to make sure I know what is going
away.  I can also edit said file, or use something like grep, if I want to
limit what is dropped.

In particular any database that isn't turned into a logical replica would
be added to this list; in addition to all the publication/subscription
stuff that is likely broken at this point.

David J.

---

From: Amit Kapila <[email-masked]>
Date: Wed, 12 Mar 2025 09:22:55 +0530

On Tue, Mar 11, 2025 at 8:21 PM David G. Johnston
<[email-masked]> wrote:
> Option 1 by far.

Among the current discussed options, I would prefer
"--remove-all-publications", or "--drop-all-publications" without any
shorthand notation. We don't need to add target as we already have
options like "--socketdir", or "--config-file" that apply to the
target server and are explained in the text. It is better to follow
the same to keep the consistency.

Also, the new subscriber can act as a publisher, so it won't be a good
idea to remove the existing copied publications by default. Similarly,
users may want to retain copied subscriptions if they want the new
target server to keep receiving the changes from previous publishers.
I feel giving individual options and then a remove-all-objects that
pre-exist on the target server would be a good direction to improve
this tool.

  Though personally I'd rather do something like what pg_upgrade does
and output a script with drop commands that can be executed via psql
instead of having pg_createsubscriber execute said commands.  I'd call
it "pruning the target".

Yes, this is a good idea, and IIRC, we discussed in brief on this in
the original thread where we developed this tool but left it as a
future improvement. We should definitely go in this direction in the
future, but let's improve this tool incrementally, otherwise, there is
a risk of making no improvements.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Wed, 12 Mar 2025 09:40:51 +0530

> "Drop publications on the target server that were replicated because they were created before the replication start location. It has no use on the subscriber."

These are the publications this tool has created on publisher to make
standby a subscriber, but they have replicated to standby before the
standby is converted to subscriber. As per my understanding, the patch
gives an option to remove publications that are pre-existing (aka not
created by this tool).

-- 
With Regards,
Amit Kapila.

---

From: "David G. Johnston" <[email-masked]>
Date: Tue, 11 Mar 2025 21:13:11 -0700

> a risk of making no improvements.

I’m against a UX that drops database objects selected by algorithm without
informing the user what those objects are and giving them a chance to
abort.  The output file method makes that very simple to implement.  I
think an interactive command to accomplish that requirement would put
commit more at risk.  And a constant increase of options for each object
type seems annoying given the alternative of simply doing them all as we
get around to them and letting the file and the user remove the ones they
wish to keep.

I’m honestly question how much value this provides - no improvement here
seems like a viable choice.  Let them issue the drop commands they desire
manually.  This doesn’t feel like something one should/would automate.

David J.

---

From: "David G. Johnston" <[email-masked]>
Date: Tue, 11 Mar 2025 21:22:47 -0700

On Tuesday, March 11, 2025, Amit Kapila <[email-masked]> wrote:

> > "Drop publications on the target server that were replicated because
> they were created before the replication start location. It has no use on
> the subscriber."
> >
>

Thank you.  I’ll factor that into my suggested changes patch for the
reference page.

David J.

---

From: Amit Kapila <[email-masked]>
Date: Wed, 12 Mar 2025 10:04:49 +0530

On Wed, Mar 12, 2025 at 9:43 AM David G. Johnston
<[email-masked]> wrote:
>
> I’m honestly question how much value this provides - no improvement here seems like a viable choice.  Let them issue the drop commands they desire manually.  This doesn’t feel like something one should/would automate.
>

There is a good chance of mistakenly dropping the required objects
manually after the subscriber is converted. One can mistakenly drop
the required publication unless they have some strict rule to do it
immediately after the tool has converted standby to subscriber.
Providing the commands via file is a way, but as a user, I would
prefer to get the things done automatically via a tool instead of me
doing it afterward unless there is no other way..

-- 
With Regards,
Amit Kapila.

---

From: "David G. Johnston" <[email-masked]>
Date: Tue, 11 Mar 2025 22:12:56 -0700

> immediately after the tool has converted standby to subscriber.

If you are referring to “step 6”, that bypasses this requirement because
pg_createsubscriber created it, knows the exact name, and it is well
defined why that specific publication should be removed.

> doing it afterward unless there is no other way..
Given that the fallback position is to restore the physical standby and do
the transformation again I suppose it isn’t too bad if we or the user
messes up a run.  But you can still get the tool to basically do it for you
automatically by just blindly sending the through psql.  The people who
want the safety net have no option.  I suspect the percentage of DBAs that
would appreciate the option would be meaningful.  After all, we felt it
necessary to add a —dry-run option.

David J.

---

From: Amit Kapila <[email-masked]>
Date: Wed, 12 Mar 2025 12:11:42 +0530

On Wed, Mar 12, 2025 at 10:42 AM David G. Johnston
<[email-masked]> wrote:
> If you are referring to “step 6”, that bypasses this requirement because pg_createsubscriber created it, knows the exact name, and it is well defined why that specific publication should be removed.
>

I am not referring to step 6. I am saying that users could have
created the publications by themselves after converting the standby to
subscriber. It won't be convenient to remember those and distinguish
them from the existing publications as there won't be any particular
pattern. The point is that there is a chance of misuse or data loss
with manual steps, which is not there if done via this option.

> Given that the fallback position is to restore the physical standby and do the transformation again I suppose it isn’t too bad if we or the user messes up a run.
>

As mentioned above, there is always a chance of data loss if it is
done manually, even if we restore to a backup of physical standby.

  But you can still get the tool to basically do it for you
automatically by just blindly sending the through psql.  The people
who want the safety net have no option.  I suspect the percentage of
DBAs that would appreciate the option would be meaningful.  After all,
we felt it necessary to add a —dry-run option.

This could be another option, but I feel an option with a tool is more
meaningful than allowing users to do afterward steps.

-- 
With Regards,
Amit Kapila.

---

From: "Euler Taveira" <[email-masked]>
Date: Thu, 13 Mar 2025 18:36:22 -0300

> meaningful than allowing users to do afterward steps.

I wasn't paying much attention to this discussion. The thread title refers to a
general option to clean publisher objects which includes non specified objects.
I was expecting a general solution but it seems to include only publications.
Why? I envision in the future adding an option to publish only a set of tables.
Will this proposal remove tables that were not published and its dependent
objects (data types, functions, ...)? When we add the initial schema
synchronization for logical replication, will this proposal be aligned with it?

It is a mistake not to explore a general solution because you risk shooting
yourself in the foot. If we consider that (a) the start point is a standby
(physical copy) and (b) in most scenarios to setup a logical replica you use
pg_dump that dumps the publications by default, it seems these additional
objects will be expected by the user.

I have some questions, fixes and concerns about this patch.

-c has nothing to do with the long option. As Amit suggested in a previous
email, it is perfectly fine to not include a short option here.

static void setup_subscriber(struct LogicalRepInfo *dbinfo,
-                            const char *consistent_lsn);
+                            const char *consistent_lsn,
+                            bool drop_all_publications);

Isn't it better to add this extra option into LogicalRepInfo? It avoids
changing this function signature and the following one.

By the way, why did you rename the function? It still removes the publication
by name, although, the function name does not contain "_by_name". The proposed
signature also has redundant argument usage.

-                   drop_publication(conn, &dbinfos.dbinfo[i]);
+                   drop_publication_by_name(conn, dbinfos.dbinfo[i].pubname,
+                                            &dbinfos.dbinfo[i]);

Even with this proposal, the following change is imprecise. It is still
removing publications (plural form). If you still want to include the fact that
pre existing publications will also be removed, say "Remove publications
(including pre existing ones)".

        /*
-        * 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.
+        * 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.
         */

-drop_publication(PGconn *conn, struct LogicalRepInfo *dbinfo)
+drop_publication_by_name(PGconn *conn, const char *pubname,
+                        struct LogicalRepInfo *dbinfo)

Keep the current function name.

-   char       *pubname_esc;
-
-   Assert(conn != NULL);

-   pubname_esc = PQescapeIdentifier(conn, dbinfo->pubname, strlen(dbinfo->pubname));
+   char       *pubname_esc = PQescapeIdentifier(conn, pubname, strlen(pubname));

Why did you remove the assert?

-   PQfreemem(pubname_esc);
-

Why did you reallocate this function call?

+/*
+ * Check and drop existing publications on the subscriber.
+ */
+static void
+check_and_drop_existing_publications(PGconn *conn, struct LogicalRepInfo *dbinfo)

AFAICS there is no "check" in this function. I would go with
drop_all_publications().

Why did you rename node_s? You just increased the number of hunks someone needs
to review. If you need a new standby, fine. Create another node: node_u.

+$node_s2->init_from_backup($node_p, 'backup_3', has_streaming => 1);
+$node_s2->append_conf(
+   'postgresql.conf', qq[
+       primary_conninfo = '$pconnstr'
+       max_logical_replication_workers = 5
+       ]);
+$node_s2->set_standby_mode();
+$node_s2->start;

Do you really need to create another node just to confirm the current behavior?
I think you can inspect the logfile after a dry run mode to see if some message
pops up. This addition increases the pg_createsubscriber runtime from

Files=1, Tests=37, 10 wallclock secs ( 0.02 usr  0.00 sys +  1.41 cusr  1.67 csys =  3.10 CPU)

to

Files=1, Tests=43, 16 wallclock secs ( 0.03 usr  0.01 sys +  1.70 cusr  2.10 csys =  3.84 CPU)

That's unacceptable.

I'm still concerned about the fact that we are adding one option that is
specific and have to add one per object as soon as someone has another
proposal. We need to decide if we want multiple options to clean up objects in
the future or a general option that will be incrementally adding objects to
remove. Multiple options are more granular and can avoid breaking backward
compatibility (if you don't want) but can increase the list of options you need
to inform if you want to clean multiple object types. A single option is not
flexible and breaks backward compatibility but it does exactly what you want
with a few characters. In most scenarios, if you want to have a clean
subscriber, you will remove *all* possible objects, hence, a single option is
your choice.

--
Euler Taveira
EDB   https://www.enterprisedb.com/

---

From: Amit Kapila <[email-masked]>
Date: Fri, 14 Mar 2025 10:56:49 +0530

On Fri, Mar 14, 2025 at 3:06 AM Euler Taveira <[email-masked]> wrote:
> objects (data types, functions, ...)?
>

We can add new publications via tool or once the subscriber is
created. This option and any other options are to remove objects that
are no longer needed from the previous set up of standby. One is
publications present which user may or may not need but it is
difficult to distinguish the publications copied from primary before
we have a  subscriber. Now, the other things could be replication
slots or probably even databases (if user consider subscriber to
create subscriber from a specified set of databases). The idea to make
this solution general is that we provide switches like the current one
for different objects and then a common switch to remove all
pre-existing objects (like --remove-all-existing-objects). I am not
sure a generic switch like --remove-all-existing-objects is good
enough because users may want to retain few pre-existing objects like
subscriptions so that the new subscriber continure to get data from
other publishers.

 When we add the initial schema
> synchronization for logical replication, will this proposal be aligned with it?

Can you please explain this a bit more to state what you have in mind for this?

> objects will be expected by the user.

It is possible and that is why we are giving it as an option rather
than removing publications or other objects by default.

> subscriber, you will remove *all* possible objects, hence, a single option is

I agree that we need a single option, but I feel we need granular
options as well to allow users to retain objects selectively.

-- 
With Regards,
Amit Kapila.

---

From: "David G. Johnston" <[email-masked]>
Date: Thu, 13 Mar 2025 23:39:22 -0700

> "--remove-target-objects=publications,subscriptions" etc.

I’m changing my vote to option 3.  With a requirement that dropping is done
post-recovery by the user via psql - we just provide the commands we would
have executed in a script.

—prune-file=‘drop-commands.psql’
—prune=publications
—prune=subscriptions
Etc…

I’d rather do multiple specifications than commas separated.  It matches
what we already do with databases, which was done this way I suspect for
the same reasons - length of the parameters value when we have:
Publications
Slots
Subscriptions
Databases
Roles
Tablespaces

Redefining All over the course of years as we decide to add object types is
unappealing.  Even if we expect the DBA to check the drop-commands.sql file
before executing it.  Which I’d still require them to do instead of
providing CLI options to specify, e.g., slot names or database names to not
drop if they want some subset of All.

David J.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Fri, 14 Mar 2025 07:25:54 +0000

On 2025/03/14 15:25:00  </o=ExchangeLabs/ou=Exchange Administrative Group (FYDIBOHF23SPDLT)/cn=Recipients/cn=3da34e28d9d240d4abadbbb549f9ff21-houzj.fnst> wrote:
On Wed, Mar 12, 2025 at 12:13 PM David G. Johnston <[email-masked]> wrote:
> On Tuesday, March 11, 2025, Amit Kapila <[email-masked] <mailto:[email-masked]> > wrote:
> > <[email-masked] <mailto:[email-masked]> > wrote:
> > >
> > > On Tue, Mar 11, 2025 at 12:20 AM Peter Smith <[email-masked] <mailto:[email-masked]> > wrote:
> > >>
> > >>
> informing the user what those objects are and giving them a chance to abort.
> The output file method makes that very simple to implement.  I think an
> interactive command to accomplish that requirement would put commit more at
> risk.  And a constant increase of options for each object type seems annoying
> given the alternative of simply doing them all as we get around to them and
> letting the file and the user remove the ones they wish to keep.
>  
> I’m honestly question how much value this provides - no improvement here seems
> like a viable choice.  Let them issue the drop commands they desire manually.
> This doesn’t feel like something one should/would automate.

For logical replication, there is a frequent need to automatically delete all
objects (including publications) on replicas that are no longer needed. This
requirement comes from a common use case in logical replication, which is to
replicate only a subset of database objects. Personally, I am uncertain about
the use case for retaining only some of the publications while dropping others.

Besides, I am unsure if the rationale behind pg_upgrade's output script is
applicable here. AFAIK, The scripts output by pg_upgrade, such as
reindex_hash.sql and update_extensions.sql are to address major version
differences. The script delete_old_cluster.sh is generated instead of directly
removing the instance to provide an option to revert if any issues arise during
the upgrade.

I think the option proposed in this thread is not to handle the
version mismatch between the publisher and the subscriber. The primary goal of
pg_createsubscriber is simply to create a subscriber. Even if problems occur
after dropping the publication, creating another subscriber using the existing
publisher is straightforward and we can't get back to standby whether we remove
publications or not. This concern is minimized since we do not delete
publications by default (only when --drop-xx option is specified). Overall,
I favor automatically deleting publications rather than offering a script for
manual execution.

Best Regards,
Hou zj

---

From: "David G. Johnston" <[email-masked]>
Date: Fri, 14 Mar 2025 08:26:32 -0700

On Fri, Mar 14, 2025 at 12:26 AM Zhijie Hou (Fujitsu) <
[email-masked]> wrote:

> replicate only a subset of database objects.

I don't see us implementing an API to provide an alternative to "ALL
TABLES"...

Personally, I am uncertain about
> the use case for retaining only some of the publications while dropping

To assume there is nothing between All or None seems unwise even if we
cannot imagine what that may be.

> applicable here.

Yeah, I probably shouldn't have mentioned it.  I don't need it to explain
or support my case.

> version mismatch between the publisher and the subscriber.

Certainly not given that this requires a physical standby as a base and
those are not cross-major-version capable.

Overall,
> I favor automatically deleting publications rather than offering a script
I'm done arguing the counter-point to this.  I cannot give this a +1 unless
the DBA is given a chance to review and edit the drop commands that we
create.  But as of now I'm in the minority and this has a committer willing
to proceed without this capability.  I take some comfort in that it seems
at least if they use --dry-run they can see what objects will be dropped.
But the editor capability seems more useful.  There is always v19 and
beyond; at least nothing here precludes adding such a feature in the future.

David J.

---

From: "David G. Johnston" <[email-masked]>
Date: Fri, 14 Mar 2025 17:42:29 -0700

> been posted since you said that.
I don't see the point of renaming S to S1.

I also don't get re-defining the existing S tests to include
--drop-all-publications and then go and add a new test that excludes
--drop-all-publications.

Just name the new subscriber D (or pick some other random letter, it isn't
like we are encouraging meaningful variable names here) and have it test
the behavior of --drop-all-publications.  Given the existing design, the
introduction of two new user publications on P initially shouldn't impact
existing tests (and if it does, seeing that change in the existing tests
would be a good thing).  An extra check or two against S should suffice to
prove non-deletion.

In short, this probably needn't touch any existing tests, just the shared
setup.

Also, can we explain why it is important to use --verbose --verbose?
Obviously it shows more information, that what repeating --verbose is
defined to do. (Sure, that isn't new to this patch.)  But why is it
important that this specific command use it when none of the others do?

David J.

---

From: Peter Smith <[email-masked]>
Date: Sat, 15 Mar 2025 16:52:17 +1100

On Fri, Mar 14, 2025 at 5:39 PM David G. Johnston
<[email-masked]> wrote:
> I’m changing my vote to option 3.  With a requirement that dropping is done post-recovery by the user via psql - we just provide the commands we would have executed in a script.
>
> I’d rather do multiple specifications than commas separated.  It matches what we already do with databases, which was done this way I suspect for the same reasons - length of the parameters value when we have:
> Publications
> Slots
> Subscriptions
> Databases
> Roles
> Tablespaces
>

OK. Here are some examples...

style #1:
--prune=Publications --prune=Slots --prune=Subscriptions
--prune=Databases --prune=Tablespaces

versus

style #2:
--prune=Publications,Slots,Subscriptions,Databases,Tablespaces

David, I understand you are saying that you prefer style #2 because of
the potentially cumbersome length of the value part if there are many
future object kinds (e.g.
"Publications,Slots,Subscriptions,Databases,Tablespaces" in the above
examples).

Does anyone else here have any preference about these styles they wish to voice?

> Redefining All over the course of years as we decide to add object types is unappealing.  Even if we expect the DBA to check the drop-commands.sql file before executing it.  Which I’d still require them to do instead of providing CLI options to specify, e.g., slot names or database names to not drop if they want some subset of All.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Peter Smith <[email-masked]>
Date: Sat, 15 Mar 2025 17:04:57 +1100

> > I’d rather do multiple specifications than commas separated.  It matches what we already do with databases, which was done this way I suspect for the same reasons - length of the parameters value…

(I'm sorry, my previous post included a confusing typo. Here is the correction)

style #1:

versus

style #2:

David, I understand you are saying that you prefer style #1 because of
the potentially cumbersome length of the csv value part of style #2 if
there are many future object kinds (e.g.
"Publications,Slots,Subscriptions,Databases,Tablespaces" in the above
examples).

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Amit Kapila <[email-masked]>
Date: Sat, 15 Mar 2025 11:45:58 +0530

> "Publications,Slots,Subscriptions,Databases,Tablespaces" in the above

Style-1 sounds reasonable to me, but how exactly we want to do. One
idea is to have short and long switches like -r and
--remove_exiting_object=publication. The user would be allowed to give
multiple options like -r publications -r slots, etc.

-- 
With Regards,
Amit Kapila.

---

From: "David G. Johnston" <[email-masked]>
Date: Sat, 15 Mar 2025 07:33:10 -0700

On Friday, March 14, 2025, Amit Kapila <[email-masked]> wrote:

> > > > I’m changing my vote to option 3.  With a requirement that dropping
> is done post-recovery by the user via psql - we just provide the commands
> we would have executed in a script.
> > > >
> > > > I’d rather do multiple specifications than commas separated.  It
> matches what we already do with databases, which was done this way I
> suspect for the same reasons - length of the parameters value when we have:
> > > > Publications
> > > > Slots
> > > > Subscriptions
> > > > Databases
> > > > Roles
> > > > Tablespaces
> > > >
> > >
> >

Either “existing” nor “object” are needed, a one-word long form suffices.
Drop, remove, or prune.  If we want a short form option we should choose
Remove and use -r; both D and P are already taken.

So, I marginally prefer —prune with no short-form option; followed by
—remove/-r

Communicating the semantic meaning of “prune” in the option name, we aren’t
removing all objects of the given type, tips the balance for me.  But that
can just be communicated in the description so it isn’t a strong desire.

David J.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 17 Mar 2025 08:44:17 +0530

On Sat, Mar 15, 2025 at 8:03 PM David G. Johnston
<[email-masked]> wrote:
> Either “existing” nor “object” are needed, a one-word long form suffices.  Drop, remove, or prune.  If we want a short form option we should choose Remove and use -r; both D and P are already taken.
>
> So, I marginally prefer —prune with no short-form option; followed by —remove/-r
>

I am inclined towards "--remove/-r" as that will be relatively more
straightforward to follow for users.

> Communicating the semantic meaning of “prune” in the option name, we aren’t removing all objects of the given type, tips the balance for me.  But that can just be communicated in the description so it isn’t a strong desire.
>

BTW, with this option, we will be removing all the publications
present on the subscriber because on standby there shouldn't be any
more. But that may not be true for other objects, so we must
communicate it via the description.

-- 
With Regards,
Amit Kapila.

---

From: Shubham Khanna <[email-masked]>
Date: Mon, 17 Mar 2025 18:04:23 +0530

> communicate it via the description.

I have incorporated the "--remove/-r" parameter in the attached patch,
as it seems more intuitive and straightforward for users.
The attached patch contains the latest changes.

Thanks and regards,
Shubham Khanna,

---

From: "Euler Taveira" <[email-masked]>
Date: Mon, 17 Mar 2025 20:01:25 -0300

> The attached patch contains the latest changes.

There were a lot of discussion around the single vs multiple options since my
last review [1] so I'm answering some of the questions here.

Due to the nature of removing multiple objects, I would say that a general
option that has a value and can be informed multiple times is the way to go. I
saw that the discussion led to this. Regarding the name, my preference is
--drop since we already have other binaries with similar options (pg_receivewal
and pg_recvlogical have --drop-slot).

The commit message needs some adjustments. There are redundant information (1st
and last paragraph).

+       <literal>publications</literal>. This option is useful when
+       transitioning from streaming replication to logical replication as
+       existing objects may no longer be needed. Before using this option,

Use physical replication. That's what we used in the current
pg_createsubscriber documentation and it is also the terminology referred in
the glossary (see Replication).

    bool        two_phase;      /* enable-two-phase option */
+   uint32      remove_objects; /* flag to remove objects on subscriber */

uint32 -> bits32.

-static void setup_subscriber(struct LogicalRepInfo *dbinfo,
-                            const char *consistent_lsn);
+static void setup_subscriber(const char *consistent_lsn);

This is unrelated to this patch.

- * replication setup.
+ * replication setup. If 'remove' parameter is specified, 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(const char *consistent_lsn)

There is no parameter 'remove' in this function. I understand that you want to
provide information about the remove option but it is not the right place to
add it. If for some reason you need to change or remove such parameter, it is
easier to left this comment because it is not near the place you are removing
some lines of code.

+       struct LogicalRepInfo *dbinfo = &dbinfos.dbinfo[i];
 
        /* Connect to subscriber. */
-       conn = connect_database(dbinfo[i].subconninfo, true);
+       conn = connect_database(dbinfo->subconninfo, true);

This new dbinfo pointer is just a way to increase your patch size without
improving readability.

-       drop_publication(conn, &dbinfo[i]);
+       if (dbinfos.remove_objects & OBJECT_PUBLICATIONS)
+           drop_all_publications(conn, dbinfo);
+       else
+           drop_publication(conn, dbinfo, dbinfo->pubname);

At first glance, I didn't like this change. You inform dbinfo->pubname as a 3rd
parameter but the 2nd parameter is dbinfo. After reading
drop_all_publication(), I realized that's the cause for this change. Is there a
better way to do it?

+static void
+drop_all_publications(PGconn *conn, struct LogicalRepInfo *dbinfo)
+{
+   PGresult   *res;
+

I would add an Assert(conn != NULL) here to follow the same pattern as the
other functions.

+   res = PQexec(conn, "SELECT pubname FROM pg_catalog.pg_publication;");
+   if (PQresultStatus(res) != PGRES_TUPLES_OK)
+   {
+       pg_log_error("could not obtain publication information: %s",
+                    PQresultErrorMessage(res));
+       PQclear(res);
+       return;
+   }

Shouldn't it disconnect_database() here? See the pattern in other functions
that error out while querying the catalog.

+       SimpleStringListCell *cell;
+
+       for (cell = opt.remove_objects.head; cell; cell = cell->next)
+       {

You could use SimpleStringListCell in the for loop without a separate declaration.

+               pg_log_error("wrong remove object is specified");
+               pg_log_error_hint("Only \"publications\" can be removed.");
+               exit(1);

The main message sounds strange. Did you mean 'wrong object is specified'?

+ok($node_s->safe_psql($db1, "SELECT COUNT(*) = 2 FROM pg_publication"),
+   'two publications created before --remove is run');
+

two pre-existing publications on subscriber ?

+is($node_s->safe_psql($db1, "SELECT COUNT(*) FROM pg_publication;"),
+   '0', 'all publications dropped after --remove is run');
+

all publications on subscriber have been removed ?

Do you need to set m_l_r_w here?

This comment is superfluous. Remove it.

+# Confirm publications still remain after running 'pg_createsubscriber'
+is($node_u->safe_psql($db3, "SELECT COUNT(*) FROM pg_publication;"),
+   '2', 'all publications remain after running pg_createsubscriber');

I would remove the semicolon here because none of the SQL commands in this test
file includes it.

Files=1, Tests=43, 14 wallclock secs ( 0.04 usr  0.01 sys +  1.65 cusr  2.06 csys =  3.76 CPU)
Result: PASS

Do we really need this extra test? It increases the test duration by 4 seconds.
Again, that's still unacceptable for such an optional feature. Maybe you should
experiment my suggestion in the previous review. You don't need a real run to
prove that pg_createsubscriber is not removing unintended objects.

[1] http://postgr.es/m/[email-masked]

---

From: "David G. Johnston" <[email-masked]>
Date: Mon, 17 Mar 2025 17:17:46 -0700

> Regarding the name, my preference is

A short form seems desired here and we cannot use -d/-D.  Also, the "act
and quit" nature of the command-like options in those two client
applications leads me to believe that this server application modifier-like
option, which behaves differently than a simple "drop named object and
return", should not have the same naming as those others.

We are not dropping named objects - the wording "Remove all given objects"
is incorrect.

"Remove all objects of the specified type from specified databases on the
target server."

"Multiple object types can be specified by writing multiple --remove
switches." (accepting switches instead of options pending bulk change)

More changes of this sort are needed.

> drop_all_publication(), I realized that's the cause for this change. Is

I had the same impression.  I'm inclined to accept it as-is and let whoever
writes the next --remove object type implementation deal with cleaning it
up.  This is clear enough when talking only about publications since
whether you just remove the one or all of them the special one we created
goes away.

> The main message sounds strange. Did you mean 'wrong object is specified'?

Error: invalid object type "%s" specified for --remove
Hint: The valid options are: "publications"

(yes, plural for a single option is "wrong", but extensible...)

If we want to just give the DBA the choice to say "all" now we could solve
two annoyances at once.

The valid options are: "all", "publications"

Should we be accepting these case-insensitively?

David J.

---

From: Shubham Khanna <[email-masked]>
Date: Tue, 18 Mar 2025 11:46:41 +0530

Fixed.

Fixed.

Fixed.

Fixed.

Fixed.

Fixed.

> drop_all_publication(), I realized that's the cause for this change. Is there a

I see your point. The use of dbinfo->pubname as the third parameter
while passing dbinfo as the second parameter does feel a bit
inconsistent. However, since drop_all_publications() relies on dbinfo
for context, this approach seemed necessary to keep the interface
consistent with drop_publication().
Currently, I do not have a better alternative that maintains clarity
and consistency, but I am open to suggestions if anyone has ideas to
improve this.

Fixed.

Fixed.

Fixed.

Fixed.

Fixed.

Fixed.

> [1] http://postgr.es/m/[email-masked]

I have removed the additional test case and the new node (node_u) that
was added for it. This should help reduce the overall test duration
without compromising the coverage of the new feature.

The attached patch contains the suggested changes,

Thanks and regards,
Shubham Khanna.

---

From: Shubham Khanna <[email-masked]>
Date: Tue, 18 Mar 2025 11:50:29 +0530

On Tue, Mar 18, 2025 at 5:48 AM David G. Johnston
<[email-masked]> wrote:
> A short form seems desired here and we cannot use -d/-D.  Also, the "act and quit" nature of the command-like options in those two client applications leads me to believe that this server application modifier-like option, which behaves differently than a simple "drop named object and return", should not have the same naming as those others.
>
> We are not dropping named objects - the wording "Remove all given objects" is incorrect.
>
> "Remove all objects of the specified type from specified databases on the target server."
>
> "Multiple object types can be specified by writing multiple --remove switches." (accepting switches instead of options pending bulk change)
>
> I had the same impression.  I'm inclined to accept it as-is and let whoever writes the next --remove object type implementation deal with cleaning it up.  This is clear enough when talking only about publications since whether you just remove the one or all of them the special one we created goes away.
>
>
> If we want to just give the DBA the choice to say "all" now we could solve two annoyances at once.
>

I have added validation for "all" to address both issues at once.
Additionally, the option parsing is now case-insensitive for better
usability.

[1] - https://www.postgresql.org/message-id/CAHv8RjK8q%2BmWPWPh9K7CeH2tKF31vGn%2BoPV3qY7pdPCmtbjzkg%40mail.gmail.com

Thanks and regards,
Shubham Khanna.

---

From: "David G. Johnston" <[email-masked]>
Date: Mon, 17 Mar 2025 23:37:16 -0700

> I have added validation for "all" to address both issues at once.
Usage needs to be changed to refer to object types and we should try and
specify which are valid there too.

The sgml docs need to mention “all” as a valid value and what it means
(namely, if new object types are added and “all” is specified those new
types will be removed as well).  Suggest not using it in scripts.

There are still more places where “object type” is needed instead of just
“object”.  I’ll pin-point or fix tomorrow if you don’t beat me to them.

It would be good if we could get this to play nicely with —dry-run; maybe
connecting to the source for the queries instead of the target.  That would
help alleviate my issue with the current auto-drop behavior.

David J.

---

From: Amit Kapila <[email-masked]>
Date: Tue, 18 Mar 2025 12:18:54 +0530

On Tue, Mar 18, 2025 at 12:07 PM David G. Johnston
<[email-masked]> wrote:
>
> On Monday, March 17, 2025, Shubham Khanna <[email-masked]> wrote:
>>
>>
> Usage needs to be changed to refer to object types and we should try and specify which are valid there too.
>
> The sgml docs need to mention “all” as a valid value and what it means (namely, if new object types are added and “all” is specified those new types will be removed as well).  Suggest not using it in scripts.
>

Hmm, I think at this stage, we don't need to add the provision for
'all'. We can add it when more than one object 'publication' will be
allowed to be removed.

With Regards,
Amit Kapila.

---

From: Shubham Khanna <[email-masked]>
Date: Tue, 18 Mar 2025 16:01:36 +0530

Fixed.

> The sgml docs need to mention “all” as a valid value and what it means (namely, if new object types are added and “all” is specified those new types will be removed as well).  Suggest not using it…

As suggested by Amit in [1], I have removed the provision for "all" as
a valid option for --remove. This keeps the implementation focused on
the current supported object type (publications).

> There are still more places where “object type” is needed instead of just “object”.  I’ll pin-point or fix tomorrow if you don’t beat me to them.
>

I identified and fixed a few instances where "object type" was needed
instead of just "object." Please let me know if I missed any other
occurrences.

> It would be good if we could get this to play nicely with —dry-run; maybe connecting to the source for the queries instead of the target.  That would help alleviate my issue with the current auto-drop behavior.
>

In the v18 patch attached at [2], I have removed the second test that
verified publications are not removed when --remove is not specified.
Now, the test suite only verifies that pg_createsubscriber with
--remove correctly removes publications from the subscriber node. This
reduces redundancy while still validating the core functionality of
the --remove option.
IIUC, for testing with --dry-run, we can directly check the relevant
stdout logs (e.g., "dropping publication 'test_pub1' ...") to verify
the call without actually dropping the publications.
However, IMO, using --dry-run alone would miss code coverage for the
actual drop publication execution part.
Please let me know if I misunderstood your point. I will update this
accordingly once I have more clarity.

[1] - https://www.postgresql.org/message-id/CAA4eK1JHD0fmyMkTe_y84gJ--WWPLVFo6kJMNxvFdcDs7nXjbQ%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CAHv8RjK8q%2BmWPWPh9K7CeH2tKF31vGn%2BoPV3qY7pdPCmtbjzkg%40mail.gmail.com

Thanks and regards,
Shubham Khanna.

---

From: Amit Kapila <[email-masked]>
Date: Tue, 18 Mar 2025 17:17:25 +0530

...
> actual drop publication execution part.

I don't understand --dry-run part of conversation here. As per
existing code (or with the patch), we seem to be already printing the
publications to be dropped in dry-run mode.

*
- drop_publication(conn, &dbinfo[i]);
+ if (dbinfos.remove_objects & OBJECT_PUBLICATIONS)
+ drop_all_publications(conn, &dbinfo[i]);
+ else
+ drop_publication(conn, &dbinfo[i], dbinfo[i].pubname);

I was expecting a call like the existing function
check_and_drop_existing_subscriptions(). The new function should check
if the user requested to remove the publication then it should query
the publisher, otherwise, just remove the one specified by dbinfo.
Also, the core drop_publication() function should take the required
parameters instead of dbinfo after this patch. That would simplify the
code.

-- 
With Regards,
Amit Kapila.

---

From: "David G. Johnston" <[email-masked]>
Date: Tue, 18 Mar 2025 13:40:36 -0700

> publications to be dropped in dry-run mode.

Sorry, that was me making a bad assumption rather than checking first.

I'm still bothered by the last paragraph of the commit message saying to
backup these publications when the preceding one claims strongly and
unconditionally that they are redundant and serve no purpose.  See my doc
changes below (the docs, whatever the form, should suffice for motivating
this feature and trying to explain it again in the commit message is
redundant).

#define OBJECTTYPE_PUBLICATIONS  0x1

- SimpleStringList remove_objects; /* list of object types to remove */
+ SimpleStringList objecttypes_to_remove; /* list of object types to remove
*/

- bits32 remove_objects; /* flag to remove objects on subscriber */
+bits32 objecttypes_to_remove; /* flags indicating which object types to
remove on subscriber */

printf(_("  -r, --remove=OBJECTTYPE         remove all objects of the
specified type from specified\n"
"                                  databases on the subscriber; accepts:
publications\n"));

+ * Drop the specified publication of the given database.  s/of/in/ ?
publication in the given database

* Drop all publications on the database.  s/on/in ? publications in the
given database

sgml:
How about this flow?

<para>
Remove all objects of the specified type from specified databases on the
target server. Multiple object types can be specified by using multiple
<option>--remove</option> switches.
</para>
<para>
publications: The "all tables" publications established for this subscriber
are
always removed; specifying this object type causes all other publications
replicated from the source server to be dropped as well.
</para>
<para>
The objects selected to be dropped are individually logged and do show
up in a --dry-run.  There is no opportunity to affect or stop the dropping
of the selected objects so consider taking a backup of them using pg_dump.
</para>

Just add more paragraphs next to "publications:" as we add more object
types.

David J.

---

From: Amit Kapila <[email-masked]>
Date: Wed, 19 Mar 2025 09:06:01 +0530

On Wed, Mar 19, 2025 at 2:11 AM David G. Johnston
<[email-masked]> wrote:
> I'm still bothered by the last paragraph of the commit message saying to backup these publications when the preceding one claims strongly and unconditionally that they are redundant and serve no purpose.
>

Agreed. I suggest we remove that part of the paragraph (Users should
back up any manually created publications before running this command.
By default, publications are preserved to avoid unintended data loss.)
from the commit message.

-- 
With Regards,
Amit Kapila.

---

From: Shubham Khanna <[email-masked]>
Date: Wed, 19 Mar 2025 10:38:35 +0530

> parameters instead of dbinfo after this patch. That would simplify the

I have updated the function drop_all_publications() to
check_and_drop_publications(), which now checks if the user requested
to remove the publication; if so, it queries the publisher. Otherwise,
it removes the one specified by dbinfo. Additionally, the
drop_publication() function now takes the required parameters directly
instead of dbinfo, simplifying the code.

Thanks and regards,
Shubham Khanna.

---

From: Shubham Khanna <[email-masked]>
Date: Wed, 19 Mar 2025 10:41:38 +0530

On Wed, Mar 19, 2025 at 2:11 AM David G. Johnston
<[email-masked]> wrote:
> I'm still bothered by the last paragraph of the commit message saying to backup these publications when the preceding one claims strongly and unconditionally that they are redundant and serve no purpose.  See my doc changes below (the docs, whatever the form, should suffice for motivating this feature and trying to explain it again in the commit message is redundant).
>

Fixed.

Fixed.

> + SimpleStringList objecttypes_to_remove; /* list of object types to remove */
>

Fixed.

> +bits32 objecttypes_to_remove; /* flags indicating which object types to remove on subscriber */
>

Fixed.

> printf(_("  -r, --remove=OBJECTTYPE         remove all objects of the specified type from specified\n"
> "                                  databases on the subscriber; accepts: publications\n"));
>

Fixed.

> + * Drop the specified publication of the given database.  s/of/in/ ?  publication in the given database
>

Fixed.

> * Drop all publications on the database.  s/on/in ? publications in the given database
>

Fixed.

> publications: The "all tables" publications established for this subscriber are
> Just add more paragraphs next to "publications:" as we add more object types.
>

Fixed.

[1] - https://www.postgresql.org/message-id/CAHv8RjJNE1ZvWhsgL54iPsJhomhcG%2B-SGPN8AnnwdLmWt6A44A%40mail.gmail.com

Thanks and regards,
Shubham Khanna.

---

From: Shubham Khanna <[email-masked]>
Date: Wed, 19 Mar 2025 10:43:48 +0530

> By default, publications are preserved to avoid unintended data loss.)

I have updated the commit message according to the suggestions. The
part about backing up manually created publications has been removed
to align with the statement that they are redundant and serve no
purpose.

Thanks and regards,
Shubham Khanna.

---

From: vignesh C <[email-masked]>
Date: Wed, 19 Mar 2025 12:09:07 +0530

> The attached patch contains the suggested changes.

Few comments
1)How about we change the below:
 #define        DEFAULT_SUB_PORT        "50432"
+#define        OBJECTTYPE_PUBLICATIONS  0x1

to
#define        OBJECTTYPE_PUBLICATIONS  0x0001

That way it will not require to be changed when we add more remove
options in the future.

2) Since this is a global variable, I felt this initialization is not required:
+       /* Verify the object types specified for removal from the subscriber */
+       dbinfos.objecttypes_to_remove = 0x0;
+       if (opt.objecttypes_to_remove.head != NULL)

3) Since this is a single statement, braces is not required:
+                       if (pg_strcasecmp(cell->val, "publications") == 0)
+                       {
+                               dbinfos.objecttypes_to_remove |=
OBJECTTYPE_PUBLICATIONS;
+                       }

4) Similarly here too:
+                               {
+
simple_string_list_append(&opt.objecttypes_to_remove, optarg);
+                               }

I have attached the changes for the same, if you are ok with the
changes kindly merge it.

Regards,
Vignesh

---

From: Shubham Khanna <[email-masked]>
Date: Wed, 19 Mar 2025 12:24:52 +0530

> I have attached the changes for the same, if you are ok with the

Thank you for the suggestions. I agree with the changes you have
attached — defining OBJECTTYPE_PUBLICATIONS as 0x0001 makes future
extensions easier, and removing unnecessary braces and redundant
initialization simplifies the code.

I have merged the changes and prepared the latest patch. The attached
patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 19 Mar 2025 06:59:07 +0000

Dear Shubham,

One comment for the API. Currently the patch introduces new option '--remove',
and the short term is '-r'. My suggestion is to use '-R' instead.

Background
==========
IIUC, we've planned to put outut-logs in some files on the directory [1].
This follows what pg_ugprade does.
 
In pg_uprade, -r means "retain SQL and log files even after successful completion" [2].
Normally logs would be removed after the sucessful upgrade, but they would not with
the option.

If we use -r for removing objects in pg_createsubscriber, it would be conflicted
with the short term of --retain. This is confusing for users.

[1]: https://www.postgresql.org/message-id/60b45b8a-3047-4a21-ba2a-ddb15daa638f%40eisentraut.org
[2]: https://www.postgresql.org/docs/devel/pgupgrade.html

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Wed, 19 Mar 2025 07:14:14 +0000

> patch contains the suggested changes.

Thanks for updating the patch. Here are few comments:

1.
					pg_log_error("object type \"%s\" is specified more than once for --remove", optarg);
					exit(1);

Consider using pg_fatal for simplicity.

2.

+		/* Fetch all publication names */
+		res = PQexec(conn, "SELECT pubname FROM pg_catalog.pg_publication;");
+		if (PQresultStatus(res) != PGRES_TUPLES_OK)
+		{
+			pg_log_error("could not obtain publication information: %s",
+						 PQresultErrorMessage(res));
+			PQclear(res);
+			disconnect_database(conn, false);
+			return;
+		}

I think we should exit here for consistency, as performed in similar cases.

3.

+		pg_log_info("dropped all publications in database \"%s\"", dbinfo->dbname);

This message may be misleading if some publications were not dropped
successfully, as drop_publication does not exit on a drop failure.

4.

	if (opt.remove_objects.head != NULL)
	{
		for (SimpleStringListCell *cell = opt.remove_objects.head; cell; cell = cell->next)
		{

I think the first null test is redundant.

I have attached a patch with the proposed changes. If you agree with these
modifications, please merge them.

Best Regards,
Hou zj
From ca4a2950ceb62709e5642eb2dbf05b08f4d2220d Mon Sep 17 00:00:00 2001
From: Hou Zhijie <[email-masked]>
Date: Wed, 19 Mar 2025 15:10:26 +0800
Subject: [PATCH] Some fixes

---
 src/bin/pg_basebackup/pg_createsubscriber.c | 26 +++++++--------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index d0e637d041f..1d31a7ebae2 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -1742,8 +1742,7 @@ check_and_drop_publications(PGconn *conn, struct LogicalRepInfo *dbinfo)
 			pg_log_error("could not obtain publication information: %s",
 						 PQresultErrorMessage(res));
 			PQclear(res);
-			disconnect_database(conn, false);
-			return;
+			disconnect_database(conn, true);
 		}
 
 		/* Drop each publication */
@@ -1752,7 +1751,6 @@ check_and_drop_publications(PGconn *conn, struct LogicalRepInfo *dbinfo)
 							 &dbinfo->made_publication);
 
 		PQclear(res);
-		pg_log_info("dropped all publications in database \"%s\"", dbinfo->dbname);
 	}
 	else
 		drop_publication(conn, dbinfo->pubname, dbinfo->dbname,
@@ -2081,10 +2079,7 @@ main(int argc, char **argv)
 				if (!simple_string_list_member(&opt.objecttypes_to_remove, optarg))
 					simple_string_list_append(&opt.objecttypes_to_remove, optarg);
 				else
-				{
-					pg_log_error("object type \"%s\" is specified more than once for --remove", optarg);
-					exit(1);
-				}
+					pg_fatal("object type \"%s\" is specified more than once for --remove", optarg);
 				break;
 			case 's':
 				opt.socket_dir = pg_strdup(optarg);
@@ -2251,18 +2246,15 @@ main(int argc, char **argv)
 	}
 
 	/* Verify the object types specified for removal from the subscriber */
-	if (opt.objecttypes_to_remove.head != NULL)
+	for (SimpleStringListCell *cell = opt.objecttypes_to_remove.head; cell; cell = cell->next)
 	{
-		for (SimpleStringListCell *cell = opt.objecttypes_to_remove.head; cell; cell = cell->next)
+		if (pg_strcasecmp(cell->val, "publications") == 0)
+			dbinfos.objecttypes_to_remove |= OBJECTTYPE_PUBLICATIONS;
+		else
 		{
-			if (pg_strcasecmp(cell->val, "publications") == 0)
-				dbinfos.objecttypes_to_remove |= OBJECTTYPE_PUBLICATIONS;
-			else
-			{
-				pg_log_error("invalid object type \"%s\" specified for --remove", cell->val);
-				pg_log_error_hint("The valid option is: \"publications\"");
-				exit(1);
-			}
+			pg_log_error("invalid object type \"%s\" specified for --remove", cell->val);
+			pg_log_error_hint("The valid option is: \"publications\"");
+			exit(1);
 		}
 	}
 
-- 
2.30.0.windows.2

---

From: Shubham Khanna <[email-masked]>
Date: Wed, 19 Mar 2025 14:02:29 +0530

> Thanks for updating the patch. Here are few comments:

Thank you for the suggestions. I agree with the changes you have attached.

> Consider using pg_fatal for simplicity.

Replacing pg_log_error with pg_fatal makes the error handling more consistent.

> I think we should exit here for consistency, as performed in similar cases.

Exiting on failure when fetching publication names improves
consistency with similar cases.

> successfully, as drop_publication does not exit on a drop failure.

Updating the log message to reflect partial drop failures avoids
misleading information.

> modifications, please merge them.

Removing the redundant NULL check simplifies the logic.

Thanks and regards,
Shubham Khanna.

---

From: Amit Kapila <[email-masked]>
Date: Wed, 19 Mar 2025 14:14:09 +0530

> and the short term is '-r'. My suggestion is to use '-R' instead.

We have discussed in the past and in this thread as well to have a
functionality like pg_upgrade to have LOG and or SQL files in which
case we may need to have an option for --retain (-r) similar to
pg_upgrade. So, using -R for this option sounds reasonable to me.

-- 
With Regards,
Amit Kapila.

---

From: Shubham Khanna <[email-masked]>
Date: Wed, 19 Mar 2025 14:31:41 +0530

> and the short term is '-r'. My suggestion is to use '-R' instead.

Changed -r to -R based on the shared analysis to avoid conflict with
the --retain option used in pg_upgrade and to maintain consistency
across tools.

The attached patch contains the suggested change.

Thanks and regards,
Shubham Khanna.

---

From: Shubham Khanna <[email-masked]>
Date: Wed, 19 Mar 2025 14:34:10 +0530

> pg_upgrade. So, using -R for this option sounds reasonable to me.

Fixed the mentioned issue by changing -r to -R to avoid conflict with
the --retain option used in pg_upgrade. The attached patch at [1]
contains the required change.

[1] - https://www.postgresql.org/message-id/CAHv8RjKbYaCRrzJzmU6gU0jt85xJftzVV-Hu9rXrkb1yFzYTtA%40mail.gmail.com

Thanks and regards,
Shubham Khanna.

---

From: vignesh C <[email-masked]>
Date: Wed, 19 Mar 2025 15:14:48 +0530

> The attached patch contains the suggested change.

Few suggestions:
1) I felt the comments from the function header of
check_and_drop_publications and the comments of the caller should be
interchanged. That will help in understanding the
check_and_drop_publications function more easily:
+                * Since the publications were created before the
consistent LSN, they
+                * remain on the subscriber even after the physical replica is
+                * promoted. Remove these publications from the
subscriber because
+                * they have no use. Additionally, if requested, drop
all pre-existing
+                * publications.
                 */
-               drop_publication(conn, &dbinfo[i]);
+               check_and_drop_publications(conn, &dbinfo[i]);

+/*
+ * Check and drop the required publications in the given database.
+ */
+static void
+check_and_drop_publications(PGconn *conn, struct LogicalRepInfo *dbinfo)

2) I was not sure if this line "Multiple object types can be specified
by using multiple --removed" is required in documentation, as
currently the only option supported is publications:
+       target server. Multiple object types can be specified by using multiple
+       <option>--remove</option> switches.
+      </para>

I felt this should be added once at least one more object type removal
is included.

Regards,
Vignesh

---

From: Shubham Khanna <[email-masked]>
Date: Thu, 20 Mar 2025 09:06:10 +0530

Fixed.

Fixed.

Thanks and regards,
Shubham Khanna.

---

From: vignesh C <[email-masked]>
Date: Thu, 20 Mar 2025 09:37:04 +0530

> The attached patch contains the suggested changes.

Couple of minor comments:
1) The second and third line comments can be in same line:
+# Create user-defined publications, wait for streaming replication to sync them
+# to the standby, then verify that '--remove'
+# removes them.

2) This comment can be in a single line:
+# In passing, also test the --enable-two-phase option and
+# --remove option

Regards,
Vignesh

---

From: Shubham Khanna <[email-masked]>
Date: Thu, 20 Mar 2025 09:45:09 +0530

Fixed.

Fixed.

Thanks and regards,
Shubham Khanna.

---

From: Amit Kapila <[email-masked]>
Date: Thu, 20 Mar 2025 17:14:54 +0530

On Thu, Mar 20, 2025 at 9:45 AM Shubham Khanna
<[email-masked]> wrote:

The patch had a bug in dry-run mode such that it was not emitting the
drop-related command for publications created by the tool with the new
option. I fixed that and pushed the patch. Thanks!

-- 
With Regards,
Amit Kapila.