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 1rUGBN-009psA-JG for pgsql-hackers@arkaria.postgresql.org; Mon, 29 Jan 2024 01:11:30 +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 1rUGBK-00AerH-UX for pgsql-hackers@arkaria.postgresql.org; Mon, 29 Jan 2024 01:11:26 +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 1rUGBJ-00Aer9-6D for pgsql-hackers@lists.postgresql.org; Mon, 29 Jan 2024 01:11:26 +0000 Received: from out2-smtp.messagingengine.com ([66.111.4.26]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rUGBF-003srv-3G for pgsql-hackers@lists.postgresql.org; Mon, 29 Jan 2024 01:11:23 +0000 Received: from compute2.internal (compute2.nyi.internal [10.202.2.46]) by mailout.nyi.internal (Postfix) with ESMTP id 3D13D5C00A8; Sun, 28 Jan 2024 20:11:19 -0500 (EST) Received: from imap49 ([10.202.2.99]) by compute2.internal (MEProxy); Sun, 28 Jan 2024 20:11:19 -0500 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=fm1; t=1706490679; x=1706577079; bh=RFv/r0lEVP P+Ur+iiotAy4HDZk5Rp9OC9PdUXJa/MwY=; b=bi/qFNU8dRLa/IaqU6Pwi3hlJN jOhdwo6lkLV7huJ35n8euYUJqz5xMI+kvNyuPCMvTq97Ex65ihbCRjzCVWBth/Aw jjEr5g8ZXdA2N+9haARNtUNdYWqUCDEBtDEBJ8UEc27sYbeWNKmUaNJ7KhDsvbgo 39zAD9ySRxY2mpusK85qHcx55HWFmyGFkH47f31Y8mpCSG9KxkVZ8ROULzV3M+HU Znj71ijYCmfaKztij4EvCZjJqsrgf+TOHyntqBGpmetIWrvOpeDK0W2qhLyQW7bE QrYWMRfx6iV0r5C4nysMOcWyXMW7CDlS34BqaCoS7MZWMzRgniZ2ZLS4yaKQ== 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-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm3; t=1706490679; x=1706577079; bh=RFv/r0lEVPP+Ur+iiotAy4HDZk5R p9OC9PdUXJa/MwY=; b=PnFnHuNaADxu6BR96RFK6DAVntO3XVNezGTp+qQDgkAV G1con32aHN0yaw7Dt+Fi24rIuZR8Pn+xXp6ZH58laEJAEuBV+rEZvbsYX0PqUStR WRm5A0Mlzg3CYzfhQB3Gv8IEKuNK4YqK17WN9XQfTUgnPxeI8LHfGlAiYP6SHJQK OkR+Zv/s7FmkGy7EUOS9URW39utTIxzj/9WhX+6gx78x1Mk2znDhKMPT+q8tmg8/ 0lOSDpEOIsQHFIYLK//4VyL435auDtceuXLggmxc9riVwsguSK872cFrcBoPFcnQ x7M1pDBGZmMrc7fhU2Ft0emvltRSHdvBlJSYIi5BTw== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvkedrfedtfedgfeduucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepofgfggfkjghffffhvfevufgtsegrtderreerredtnecuhfhrohhmpedfgfhu lhgvrhcuvfgrvhgvihhrrgdfuceovghulhgvrhesvghulhgvrhhtohdrtghomheqnecugg ftrfgrthhtvghrnhepgfffjedviedvkedviedvteduieekudeivedthffhuddvteegleet ieffkeeghfdvnecuffhomhgrihhnpegvnhhtvghrphhrihhsvggusgdrtghomhenucevlh hushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpegvuhhlvghrsegv uhhlvghrthhordgtohhm X-ME-Proxy: Feedback-ID: i0c21471d:Fastmail Received: by mailuser.nyi.internal (Postfix, from userid 501) id ACEB215A0092; Sun, 28 Jan 2024 20:11:18 -0500 (EST) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.11.0-alpha0-119-ga8b98d1bd8-fm-20240108.001-ga8b98d1b MIME-Version: 1.0 Message-Id: In-Reply-To: References: <5ac50071-f2ed-4ace-a8fd-b892cffd33eb@www.fastmail.com> <4710d895-689a-4ae6-b8e3-344286dbf970@app.fastmail.com> <96e3b739-d63d-4d35-90a3-7869465b7167@eisentraut.org> <2736443c-58e3-42df-8cc4-8c66106e3da5@app.fastmail.com> Date: Sun, 28 Jan 2024 22:10:45 -0300 From: "Euler Taveira" To: "kuroda.hayato@fujitsu.com" , "pgsql-hackers@lists.postgresql.org" Cc: "vignesh C" , "Michael Paquier" , "Peter Eisentraut" , "Andres Freund" , "Ashutosh Bapat" , "Amit Kapila" , "Shlok Kyal" Subject: Re: speed up a logical replica setup Content-Type: multipart/alternative; boundary=4cb31523cce7428a9eebcb757a3e50d7 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --4cb31523cce7428a9eebcb757a3e50d7 Content-Type: text/plain On Fri, Jan 26, 2024, at 4:55 AM, Hayato Kuroda (Fujitsu) wrote: > Again, thanks for updating the patch! There are my random comments for v9. Thanks for checking v9. I already incorporated some of the points below into the next patch. Give me a couple of hours to include some important points. > 01. > I cannot find your replies for my comments#7 [1] but you reverted related changes. > I'm not sure you are still considering it or you decided not to include changes. > Can you clarify your opinion? > (It is needed because changes are huge so it quite affects other developments...) It is still on my list. As I said in a previous email I'm having a hard time reviewing pieces from your 0002 patch because you include a bunch of things into one patch. > 02. > ``` > + > + > ``` > > But source codes required `--recovery-timeout`. Please update either of them, Oops. Fixed. My preference is --recovery-timeout because someone can decide to include a --timeout option for this tool. > 03. > ``` > + * Create a new logical replica from a standby server > ``` > > Junwang pointed out to change here but the change was reverted [2] > Can you clarify your opinion as well? I'll review the documentation once I fix the code. Since the tool includes the *create* verb into its name, it seems strange use another verb (convert) in the description. Maybe we should just remove the word *new* and a long description explains the action is to turn the standby (physical replica) into a logical replica. > 04. > ``` > +/* > + * Is the source server ready for logical replication? If so, create the > + * publications and replication slots in preparation for logical replication. > + */ > +static bool > +setup_publisher(LogicalRepInfo *dbinfo) > ``` > > But this function verifies the source server. I felt they should be in the > different function. I split setup_publisher() into check_publisher() and setup_publisher(). > 05. > ``` > +/* > + * Is the target server ready for logical replication? > + */ > +static bool > +setup_subscriber(LogicalRepInfo *dbinfo) > ```` > > Actually, this function does not set up subscriber. It just verifies whether the > target can become a subscriber, right? If should be renamed. I renamed setup_subscriber() -> check_subscriber(). > 06. > ``` > + atexit(cleanup_objects_atexit); > ``` > > The registration of the cleanup function is too early. This sometimes triggers > a core-dump. E.g., I forgot to apply the atexit() patch. > 07. > Missing a removal of publications on the standby. It was on my list to do. It will be in the next patch. > 08. > Missing registration of LogicalRepInfo in the typedefs.list. Done. > 09 > ``` > + > + > + pg_subscriber > + option > + > + > ``` > > Can you reply my comment#2 [4]? I think mandatory options should be written. I included the mandatory options into the synopsis. > 10. > Just to confirm - will you implement start_standby/stop_standby functions in next version? It is still on my list. -- Euler Taveira EDB https://www.enterprisedb.com/ --4cb31523cce7428a9eebcb757a3e50d7 Content-Type: text/html Content-Transfer-Encoding: quoted-printable
On Fri, Jan 26,= 2024, at 4:55 AM, Hayato Kuroda (Fujitsu) wrote:
Again, thanks for updating the pa= tch! There are my random comments for v9.
Thanks for checking v9. I already incorporated some of the p= oints below into
the next patch. Give me a couple of hours= to include some important points.

01.
I cannot find y= our replies for my comments#7 [1] but you reverted related changes.
<= /div>
I'm not sure you are still considering it or you decided not t= o include changes.
Can you clarify your opinion?
=
(It is needed because changes are huge so it quite affects other de= velopments...)

It is still on = my list. As I said in a previous email I'm having a hard time
<= div>reviewing pieces from your 0002 patch because you include a bunch of= things
into one patch.

02.
```
+       <term><option>-t= <replaceable class=3D"parameter">seconds</replaceable></= option></term>
+     &nb= sp; <term><option>--timeout=3D<replaceable class=3D"param= eter">seconds</replaceable></option></term>
```

But source codes required `--rec= overy-timeout`. Please update either of them,

Oops. Fixed. My preference is --recovery-timeout because= someone can decide to
include a --timeout option for this= tool.

03.
```
+ *    Cre= ate a new logical replica from a standby server
```

Junwang pointed out to change here but the change= was reverted [2]
Can you clarify your opinion as well?

I'll review the documentation on= ce I fix the code. Since the tool includes the
*create* ve= rb into its name, it seems strange use another verb (convert) in the
=
description. Maybe we should just remove the word *new* and a= long description
explains the action is to turn the stand= by (physical replica) into a logical
replica.

04.
<= /div>
```
+/*
+ * Is the source server r= eady for logical replication? If so, create the
+ * public= ations and replication slots in preparation for logical replication.
=
+ */
+static bool
+setup_publishe= r(LogicalRepInfo *dbinfo)
```

But this function verifies the source server. I felt they should be in = the
different function.

I split setup_publisher() into check_publisher() and setup_publi= sher().

05.
```
+/*
+ * Is= the target server ready for logical replication?
+ */
=
+static bool
+setup_subscriber(LogicalRepInfo *= dbinfo)
````

Actually, this f= unction does not set up subscriber. It just verifies whether the
target can become a subscriber, right? If should be renamed.
<= /div>

I renamed setup_subscriber() = -> check_subscriber().

06.
```
+ &nb= sp;  atexit(cleanup_objects_atexit);
```

The registration of the cleanup function is too early. = This sometimes triggers
a core-dump. E.g., 
=

I forgot to apply the atexit() patch.

07.
Missing a removal of publications on the standby.

It was on my list to do. It wil= l be in the next patch.

08.
Missing registration of Logica= lRepInfo in the typedefs.list.

Done.

09
```
+ <refsynopsisdiv>
+  <cmdsynopsis>
+   <com= mand>pg_subscriber</command>
+   <ar= g rep=3D"repeat"><replaceable>option</replaceable></ar= g>
+  </cmdsynopsis>
+ </re= fsynopsisdiv>
```

Can you = reply my comment#2 [4]? I think mandatory options should be written.
=

I included the mandatory options = into the synopsis.

10.
Just to confirm - will you implemen= t start_standby/stop_standby functions in next version?

It is still on my list.


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

--4cb31523cce7428a9eebcb757a3e50d7--