public inbox for [email protected]  

help / color / mirror / Atom feed

From: Hayato Kuroda (Fujitsu) <[email protected]>
To: 'Shubham Khanna' <[email protected]>
Cc: Shlok Kyal <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Peter Smith <[email protected]>
Subject: RE: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
Date: Tue, 25 Feb 2025 11:20:28 +0000
Message-ID: <OSCPR01MB14966189AEFD119DD04FE6B42F5C32@OSCPR01MB14966.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAHv8Rj+zmkwunNubo+_Gp26S_qXbD=p+rMfEnPnjiEE1A6GTXQ@mail.gmail.com>
References: <CAHv8RjL4OvoYafofTb_U_JD5HuyoNowBoGpMfnEbhDSENA74Kg@mail.gmail.com>
	<OSCPR01MB1496664FDC38DA40A441F449FF5EE2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
	<CAHv8RjJw2xDyWgagdAL7RBUEEEZtKcVJO_gNjQQHkUj8QrmwuQ@mail.gmail.com>
	<CANhcyEUkQybOe=J5KypWQ80deULBLjjsdyP+DF57+BwSni57Vw@mail.gmail.com>
	<CAHv8Rj+1v6257anqijp90f3cdZvSOhkm_ZRQRVU1C_0OYq056g@mail.gmail.com>
	<OSCPR01MB1496679AD9C3C47DB30542ECBF5F12@OSCPR01MB14966.jpnprd01.prod.outlook.com>
	<CAHv8RjJGjA_fwJ_TNrHbyTz7ATA1MXfNxPhoBvJfKRfk1Qf-ng@mail.gmail.com>
	<CANhcyEXfn-TGW2L991vXAaGH-UVBCAZuDUSRFTyVTQPbDgamdA@mail.gmail.com>
	<CAHv8RjKGywu+3cAv9MPgxi1_TUXBT8yzUsW+f=g5UsgeJ8Uk6g@mail.gmail.com>
	<CANhcyEUo7F954LULk859xs6FtwQ5USH6C2tiBbGwpihU2yHmAQ@mail.gmail.com>
	<CAHv8Rj+=5k_i=HJBE=615CckH6qi4B1n9jzhseKSH65MMkovNQ@mail.gmail.com>
	<CANhcyEWSsx_kSY4o89Yv6UfHkTVB727OiExcB8944ez0EixJGw@mail.gmail.com>
	<CAHv8RjJhwr7BqwFhMpcjjgYk9F-=MCx10LCiff9=7=vD-cj4GA@mail.gmail.com>
	<CAHut+Pt9AidicguCX8b0etzP8_hy0RmAfgLR2wKC3cxeuz5E=w@mail.gmail.com>
	<CAHv8RjLgLCbg-e0_5Ly_-8AojraV1R1g4BuaL-an7+iTLMycLg@mail.gmail.com>
	<CAHut+PvwoLheTMkUgu_syf6cUR+-E62ChOZw9Aohq0i5soeaBw@mail.gmail.com>
	<CAHv8Rj+zmkwunNubo+_Gp26S_qXbD=p+rMfEnPnjiEE1A6GTXQ@mail.gmail.com>

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

Best regards,
Hayato Kuroda
FUJITSU LIMITED

view thread (76+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: RE: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.
  In-Reply-To: <OSCPR01MB14966189AEFD119DD04FE6B42F5C32@OSCPR01MB14966.jpnprd01.prod.outlook.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox