Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1tuJTz-007KYw-7z for pgsql-hackers@arkaria.postgresql.org; Mon, 17 Mar 2025 23:02:55 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1tuJSy-003I3F-Vw for pgsql-hackers@arkaria.postgresql.org; Mon, 17 Mar 2025 23:01:53 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1tuJSx-003I2C-MK for pgsql-hackers@lists.postgresql.org; Mon, 17 Mar 2025 23:01:52 +0000 Received: from fout-a1-smtp.messagingengine.com ([103.168.172.144]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1tuJSv-003Oxh-0W for pgsql-hackers@lists.postgresql.org; Mon, 17 Mar 2025 23:01:50 +0000 Received: from phl-compute-13.internal (phl-compute-13.phl.internal [10.202.2.53]) by mailfout.phl.internal (Postfix) with ESMTP id 1055C1380A09; Mon, 17 Mar 2025 19:01:48 -0400 (EDT) Received: from phl-imap-09 ([10.202.2.99]) by phl-compute-13.internal (MEProxy); Mon, 17 Mar 2025 19:01:48 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=eulerto.com; h= cc:cc:content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:subject :subject:to:to; s=fm2; t=1742252508; x=1742338908; bh=v5SzsFqOiB GzVGooiOZyKD1nBnLN+r4H10a2lAS+FBY=; b=poMSrPs0v9PVnbzQvTMas4oowr hulwAfmV2rVQrtCngO2+dt80glbwCpJyyiaReZzoVabtR9OvfWLlt1mH1w8QYYb2 xsmf+eJWw0jn0nEoniJeGE1WGw+khPWRBuQJMJjb5vMx2tEE90f7k4deGVW+bI1c XxGflqFBRvPw+ChR+ZFavVQVgI5Hq+VHSFSg39QnljF2cfqKlVR6PX0stgbInHvq QuLQCrhc1exKwUpsSWlL+cGc/Aid0n0WnmBywwBWVPy4LVCTD1HIZ6s993KXOEjo N4qXtwfHP2hfPt4BKGbpT0GRQEVc3b1tgrX/0kdfBLnyk7dKNOv8yBmmJrCw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:subject:subject:to :to:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t= 1742252508; x=1742338908; bh=v5SzsFqOiBGzVGooiOZyKD1nBnLN+r4H10a 2lAS+FBY=; b=XhqhxxL3AKXTrDEROfmsjiebMkEwzzRBmadYPVe+QvlVKDbyWc+ /K34+u03S7w7YjcKcn662FYkOYtD3B0n61PRhAA0oIYU6m8bb3dokKwDcWN/dC+t NEXBin2vDWmEF5N+7+X1KAK375mNb88+4u5YEcGkEZJEBD78JcNtm7aJxGHryWNw ahyg8Z/SrCe9b3KXA9K42uuHtggPS5vqC6vzHukUiU5zlw+78xCr5CnpqVRBdhpk Td8wZ0FanijzD6OcoYywBOqbdQVOG48pfYOaNU+7OziFFSv9WdZF6zlAAFtudVZ2 jHDXD4+1p9SQx4aAXMmbBQdzeg5swbG4A4w== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeefvddrtddtgddugedtkedtucetufdoteggodetrf dotffvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdggtfgfnhhsuhgsshgtrhhisggv pdfurfetoffkrfgpnffqhgenuceurghilhhouhhtmecufedttdenucesvcftvggtihhpih gvnhhtshculddquddttddmnecujfgurhepofggfffhvfevkfgjfhfutgesrgdtreerredt tdenucfhrhhomhepfdfguhhlvghrucfvrghvvghirhgrfdcuoegvuhhlvghrsegvuhhlvg hrthhordgtohhmqeenucggtffrrghtthgvrhhnpeeitdffffejjeekuefhueethfdvvdef keetteevffeijeffveejteelhefgvedvieenucffohhmrghinhepphhoshhtghhrrdgvsh dpvghnthgvrhhprhhishgvuggsrdgtohhmnecuvehluhhsthgvrhfuihiivgeptdenucfr rghrrghmpehmrghilhhfrhhomhepvghulhgvrhesvghulhgvrhhtohdrtghomhdpnhgspg hrtghpthhtohepkedpmhhouggvpehsmhhtphhouhhtpdhrtghpthhtohepkhhurhhouggr rdhhrgihrghtohesfhhujhhithhsuhdrtghomhdprhgtphhtthhopegrmhhithdrkhgrph hilhgrudeisehgmhgrihhlrdgtohhmpdhrtghpthhtohepuggrvhhiugdrghdrjhhohhhn shhtohhnsehgmhgrihhlrdgtohhmpdhrtghpthhtohepkhhhrghnnhgrshhhuhgshhgrmh duudeljeesghhmrghilhdrtghomhdprhgtphhtthhopehnihhshhgrrdhmohhonhgugedu vdesghhmrghilhdrtghomhdprhgtphhtthhopehshhhlohhkrdhkhigrlhdrohhsshesgh hmrghilhdrtghomhdprhgtphhtthhopehsmhhithhhphgsvddvhedtsehgmhgrihhlrdgt ohhmpdhrtghpthhtohepphhgshhqlhdqhhgrtghkvghrsheslhhishhtshdrphhoshhtgh hrvghsqhhlrdhorhhg X-ME-Proxy: Feedback-ID: i0c21471d:Fastmail Received: by mailuser.phl.internal (Postfix, from userid 501) id A47BB780068; Mon, 17 Mar 2025 19:01:47 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface MIME-Version: 1.0 Date: Mon, 17 Mar 2025 20:01:25 -0300 From: "Euler Taveira" To: "'Shubham Khanna'" , "Amit Kapila" Cc: "David G. Johnston" , "Peter Smith" , "Nisha Moond" , "kuroda.hayato@fujitsu.com" , "Shlok Kyal" , "PostgreSQL Hackers" Message-Id: <443f5d3d-0073-4e0a-ba99-48e3e015f5b5@app.fastmail.com> In-Reply-To: References: Subject: Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility. Content-Type: multipart/alternative; boundary=58a58bb8e4f24cd383a7b8cb3f5b4d6b List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --58a58bb8e4f24cd383a7b8cb3f5b4d6b Content-Type: text/plain Content-Transfer-Encoding: 7bit On Mon, Mar 17, 2025, at 9:34 AM, Shubham Khanna wrote: > 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. 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). + publications. 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 ? + primary_conninfo = '$pconnstr' + max_logical_replication_workers = 5 Do you need to set m_l_r_w here? +# --verbose is used twice to show more information. 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/6f266ce2-38d4-433f-afc9-b47c48c17509@app.fastmail.com -- Euler Taveira EDB https://www.enterprisedb.com/ --58a58bb8e4f24cd383a7b8cb3f5b4d6b Content-Type: text/html Content-Transfer-Encoding: quoted-printable
On Mon, Mar 17,= 2025, at 9:34 AM, Shubham Khanna wrote:
I have incorporated the "--remove/-r" para= meter in the attached patch,
as it seems more intuitive an= d straightforward for users.
The attached patch contains t= he 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 ob= jects, 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 a= lready have other binaries with similar options (pg_receivewal
and pg_recvlogical have --drop-= slot).

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

<= /div>
+       <literal>publicati= ons</literal>. This option is useful when
+       transi= tioning from streaming replication to logical replication as
+     =   existing objects may no longer be needed. Before using this optio= n,

Use physical replication. That's wha= t we used in the current
=
pg_createsubscriber documentation and it is also the terminology re= ferred in
the glossa= ry (see Replication).

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

uint32 -> bits32.

-static void setup_subscriber(struct LogicalRepInfo *dbinfo,
-   &nb= sp;           &nb= sp;            co= nst 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 s= ubscriptions.
 = */
static void
-setup_subscriber(struct= LogicalRepInfo *dbinfo, const char *consistent_lsn)
+setup_subscriber(const char *consisten= t_lsn)

There is no parameter 'remove' i= n this function. I understand that you want to
provide information about the remove option b= ut it is not the right place to
add it. If for some reason you need to change or remove such= parameter, it is
ea= sier to left this comment because it is not near the place you are remov= ing
some lines of co= de.

+     &nbs= p; struct LogicalRepInfo *dbinfo =3D &dbinfos.dbinfo[i];
 
        /= * Connect to subscriber. */
-       conn =3D connect_database(= dbinfo[i].subconninfo, true);
<= /div>
+       conn =3D connect_databas= e(dbinfo->subconninfo, true);

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

-       dr= op_publication(conn, &dbinfo[i]);
+       if (dbinfos.remo= ve_objects & OBJECT_PUBLICATIONS)
+         = ;  drop_all_publications(conn, dbinfo);
+       else
+    = ;       drop_publication(conn, dbinfo, dbi= nfo->pubname);
At first glance, I di= dn'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 chang= e. Is there a
better= way to do it?

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

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

+   res =3D PQexec(conn, "SELECT pubname FR= OM pg_catalog.pg_publication;");
+   if (PQresultStatus(res) !=3D PGRES_TUPLES_OK)=
+   {
+   &nbs= p;   pg_log_error("could not obtain publication information: %= s",
+  &nb= sp;           &nb= sp;     PQresultErrorMessage(res));
+      = ; PQclear(res);
+&nb= sp;      return;
+   }

S= houldn't it disconnect_database() here? See the pattern in other functio= ns
that error out wh= ile querying the catalog.

+  =      SimpleStringListCell *cell;
+
+       for (cell =3D opt.r= emove_objects.head; cell; cell =3D cell->next)
+       {

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

+           &n= bsp;   pg_log_error("wrong remove object is specified");
+    =            pg_log_erro= r_hint("Only \"publications\" can be removed.");
+       =         exit(1);

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

+ok($node_s->safe_ps= ql($db1, "SELECT COUNT(*) =3D 2 FROM pg_publication"),
+   'two publications creat= ed before --remove is run');
+

two pre-existing publication= s on subscriber ?
+is($node_s->safe_= psql($db1, "SELECT COUNT(*) FROM pg_publication;"),
+   '0', 'all publications dro= pped after --remove is run');
<= /div>
+

all publications on subscri= ber have been removed ?
<= div>
+  &n= bsp;    primary_conninfo =3D '$pconnstr'
+      = ; max_logical_replication_workers =3D 5

Do you need to set m_l_r_w here?

+# --= verbose is used twice to show more information.

This comment is superfluous. Remove it.

<= /div>
+# Confirm publications still remain after running 'pg_creates= ubscriber'
+is($node= _u->safe_psql($db3, "SELECT COUNT(*) FROM pg_publication;"),
+   '2', 'all publ= ications remain after running pg_createsubscriber');

I would remove the semicolon here because none of the SQL = commands in this test
file includes it.
=
Files=3D1, Tests=3D= 43, 14 wallclock secs ( 0.04 usr  0.01 sys +  1.65 cusr  = 2.06 csys =3D  3.76 CPU)
<= /div>
Result: PASS

Do we really nee= d this extra test? It increases the test duration by 4 seconds.
Again, that's still unacceptable for such an optional feature. May= be you should
experi= ment my suggestion in the previous review. You don't need a real run to<= br style=3D"max-width:100%;height:auto;">
prove that pg_create= subscriber is not removing unintended objects.




--
Euler = Taveira
EDB   https://www.enterprisedb.com/
<= /div>

--58a58bb8e4f24cd383a7b8cb3f5b4d6b--