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 1rcD4K-001aQ5-6j for pgsql-hackers@arkaria.postgresql.org; Mon, 19 Feb 2024 23:29:04 +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 1rcD4I-00HYU1-Pf for pgsql-hackers@arkaria.postgresql.org; Mon, 19 Feb 2024 23:29:02 +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 1rcD4H-00HYTR-SL for pgsql-hackers@lists.postgresql.org; Mon, 19 Feb 2024 23:29:02 +0000 Received: from fhigh5-smtp.messagingengine.com ([103.168.172.156]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rcD4E-007azg-KL for pgsql-hackers@lists.postgresql.org; Mon, 19 Feb 2024 23:29:00 +0000 Received: from compute2.internal (compute2.nyi.internal [10.202.2.46]) by mailfhigh.nyi.internal (Postfix) with ESMTP id 02C1111400D7; Mon, 19 Feb 2024 18:28:57 -0500 (EST) Received: from imap49 ([10.202.2.99]) by compute2.internal (MEProxy); Mon, 19 Feb 2024 18:28:57 -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=fm2; t=1708385336; x=1708471736; bh=eC0neswQGv yL8mH9UvJP0RC4iu3GCeZkaPBKa1EVzIo=; b=J3PiedNLI4V0HKcG7nxkyPeMsX NJ22yQz0tmymAgq4OeK3vZcmPNhLmB3o6qY2K28Abzc1nKsmiT3fEY3xNik0Oc+5 H2Y/LQfej5T3fWUxDPVX+FZDZJoCJ312yr+umNfUqbnQj/rpeB2qZajw++8j9UYx DLSjNW6XAoFd3NYLazNl34G3lDgVA5suO2Br5Zj90DccokxwA2stoXjy4dqV+PFf Bj1cLu+V4NN1/wCUM3AGGK2xgxC6+TRDtoEM4EwKfliPxIm+b0FNMbKVUu3v7dYg K4XETL+N0hpKK//fY+LufJoZvu5SIty7R3acr07AV89EhuXU6hIxe5D/x2Xg== 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= fm1; t=1708385336; x=1708471736; bh=eC0neswQGvyL8mH9UvJP0RC4iu3G CeZkaPBKa1EVzIo=; b=TZzwc42My1FnO0cTNd2jLXclieKwHxBVwxQ+jyMRRF7E DdsSnl95JFvU+S2rOvB2FmDwp/cPIA71z1fy7GHwJgGM59aBYuuuQ/ZEhFQWzQaI +D7V4F9mTciJ5gTU4pet8iGsgom+iooAAG/1GCNkJOU1TwTlM2XUVE5ODhhP+F3k AqSMS24vKDpQqG9mZWu+gs5AuzbmjrRmmZzbdrjiykJjXQlDpMQLjx1l/usqObL6 K4NBvSRS8BHTEbXQWNq3AFldNaJdBc5dHqBoMxiMc3N9bU4mawmxyEX3IZu6mSH5 o24lUpT74w8BezdMRG760zYsC1RE+XgYmxsW4VsXmA== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvledrvdelgddtlecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefofgggkfgjfhffhffvvefutgesrgdtreerreertdenucfhrhhomhepfdfguhhl vghrucfvrghvvghirhgrfdcuoegvuhhlvghrsegvuhhlvghrthhordgtohhmqeenucggtf frrghtthgvrhhnpeehvdevueeljeekfeeliefhfeefgfffgeeufefhvdeugedttdegvdeh uedvgfdugfenucffohhmrghinhepphhoshhtghhrvghsqhhlrdhorhhgpdgvnhhtvghrph hrihhsvggusgdrtghomhenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgr ihhlfhhrohhmpegvuhhlvghrsegvuhhlvghrthhordgtohhm X-ME-Proxy: Feedback-ID: i0c21471d:Fastmail Received: by mailuser.nyi.internal (Postfix, from userid 501) id 9335C15A0092; Mon, 19 Feb 2024 18:28:56 -0500 (EST) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.11.0-alpha0-144-ge5821d614e-fm-20240125.002-ge5821d61 MIME-Version: 1.0 Message-Id: <0404dab2-9475-42e3-ae39-f8a71ec1d889@app.fastmail.com> In-Reply-To: <3ee79f2c-e8b3-4342-857c-a31b87e1afda@eisentraut.org> References: <9a9c7e65-c47b-4400-a63e-ac6169c0860e@app.fastmail.com> <8d8d9960-8130-4216-b0e4-adf36b8b1c43@app.fastmail.com> <370673e8-e2da-4249-8f07-cb67e9d9c20e@app.fastmail.com> <34075bd4-c85d-4654-a782-0cd6713b2c89@app.fastmail.com> <2edde173-5ff9-45fd-b881-cd04eb13049f@app.fastmail.com> <2231a04b-f2d4-4a4e-b5cd-56be8b002427@app.fastmail.com> <3ee79f2c-e8b3-4342-857c-a31b87e1afda@eisentraut.org> Date: Mon, 19 Feb 2024 20:28:28 -0300 From: "Euler Taveira" To: "Peter Eisentraut" , "kuroda.hayato@fujitsu.com" Cc: "pgsql-hackers@lists.postgresql.org" , "vignesh C" , "Ashutosh Bapat" , "Amit Kapila" , "Shlok Kyal" , =?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= Subject: Re: speed up a logical replica setup Content-Type: multipart/alternative; boundary=2a3bece6ef59455b84179886a6ceac61 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --2a3bece6ef59455b84179886a6ceac61 Content-Type: text/plain On Mon, Feb 19, 2024, at 6:47 AM, Peter Eisentraut wrote: > Some review of the v21 patch: Thanks for checking. > - commit message > > Mention pg_createsubscriber in the commit message title. That's the > most important thing that someone doing git log searches in the future > will be looking for. Right. Done. > - doc/src/sgml/ref/allfiles.sgml > > Move the new entry to alphabetical order. Done. > > - doc/src/sgml/ref/pg_createsubscriber.sgml > > + > + The pg_createsubscriber should be run at > the target > + server. The source server (known as publisher server) should accept > logical > + replication connections from the target server (known as subscriber > server). > + The target server should accept local logical replication connection. > + > > "should" -> "must" ? Done. > + > + Options > > Sort options alphabetically. Done. > It would be good to indicate somewhere which options are mandatory. I'll add this information in the option description. AFAICT the synopsis kind of indicates it. > + > + Examples > > I suggest including a pg_basebackup call into this example, so it's > easier for readers to get the context of how this is supposed to be > used. You can add that pg_basebackup in this example is just an example > and that other base backups can also be used. We can certainly add it but creating a standby isn't out of scope here? I will make sure to include references to pg_basebackup and the "Setting up a Standby Server" section. > - doc/src/sgml/reference.sgml > > Move the new entry to alphabetical order. Done. > - src/bin/pg_basebackup/Makefile > > Move the new sections to alphabetical order. Done. > - src/bin/pg_basebackup/meson.build > > Move the new sections to alphabetical order. Done. > > - src/bin/pg_basebackup/pg_createsubscriber.c > > +typedef struct CreateSubscriberOptions > +typedef struct LogicalRepInfo > > I think these kinds of local-use struct don't need to be typedef'ed. > (Then you also don't need to update typdefs.list.) Done. > +static void > +usage(void) > > Sort the options alphabetically. Are you referring to s/options/functions/? > +static char * > +get_exec_path(const char *argv0, const char *progname) > > Can this not use find_my_exec() and find_other_exec()? It is indeed using it. I created this function because it needs to run the same code path twice (pg_ctl and pg_resetwal). > +int > +main(int argc, char **argv) > > Sort the options alphabetically (long_options struct, getopt_long() > argument, switch cases). Done. > - .../t/040_pg_createsubscriber.pl > - .../t/041_pg_createsubscriber_standby.pl > > These two files could be combined into one. Done. > +# Force it to initialize a new cluster instead of copying a > +# previously initdb'd cluster. > > Explain why? Ok. It needs a new cluster because it will have a different system identifier so we can make sure the target cluster is a copy of the source server. > +$node_s->append_conf( > + 'postgresql.conf', qq[ > +log_min_messages = debug2 > > Is this setting necessary for the test? No. It is here as a debugging aid. Better to include it in a separate patch. There are a few messages that I don't intend to include in the final patch. All of these modifications will be included in the next patch. I'm finishing to integrate patches proposed by Hayato [1] and some additional fixes and refactors. [1] https://www.postgresql.org/message-id/TYCPR01MB12077A8421685E5515DE408EEF5512%40TYCPR01MB12077.jpnprd01.prod.outlook.com -- Euler Taveira EDB https://www.enterprisedb.com/ --2a3bece6ef59455b84179886a6ceac61 Content-Type: text/html Content-Transfer-Encoding: quoted-printable
On Mon, Feb 19,= 2024, at 6:47 AM, Peter Eisentraut wrote:
Some review of the v21 patch:
<= /blockquote>

Thanks for checking.

- commit message

Mention pg_createsubscriber in the commit mes= sage title.  That's the 
most important thing th= at someone doing git log searches in the future 
will= be looking for.

Right. Done.<= /div>

= - doc/src/sgml/ref/allfiles.sgml

Move the n= ew entry to alphabetical order.

Done.


- doc/src/sgml/ref/pg_createsubscriber.sgml

+  <para>
+ &n= bsp; The <application>pg_createsubscriber</application> shou= ld be run at 
the target
+   = server. The source server (known as publisher server) should accept = ;
logical
+   replication connecti= ons from the target server (known as subscriber 
serv= er).
+   The target server should accept local l= ogical replication connection.
+  </para>

"should" -> "must" ?

Done.

+ <refsect1>
+  <tit= le>Options</title>

Sort options al= phabetically.

Done.
=
It would b= e good to indicate somewhere which options are mandatory.

I'll add this information in the option desc= ription. AFAICT the synopsis kind
of indicates it.

+ &= lt;refsect1>
+  <title>Examples</title>= ;

I suggest including a pg_basebackup call = into this example, so it's
easier for readers to get the c= ontext of how this is supposed to be
used.  You can a= dd that pg_basebackup in this example is just an example 
=
and that other base backups can also be used.

We can certainly add it but creating a standby isn'= t out of scope here? I will
make sure to include reference= s to pg_basebackup and the "Setting up a Standby
Server" s= ection.

- doc/src/sgml/reference.sgml

M= ove the new entry to alphabetical order.

=
Done.

- src/bin/pg_basebackup/Makefile

=
Move the new sections to alphabetical order.

Done.

- src/bin/pg_basebackup/meson.build<= br>

Move the new sections to alphabetical order= .

Done.


- s= rc/bin/pg_basebackup/pg_createsubscriber.c

= +typedef struct CreateSubscriberOptions
+typedef struct Lo= gicalRepInfo

I think these kinds of local-u= se struct don't need to be typedef'ed.
(Then you also don'= t need to update typdefs.list.)

Done.

+static void
+usage(void)

=
Sort the options alphabetically.

Are you referring to s/options/functions/?

<= blockquote type=3D"cite" id=3D"qt" style=3D"">
+static char *
+get_exec_path(const char *argv0, const char *progname)

Can this not use find_my_exec() and find_other_exec= ()?

It is indeed using it. I c= reated this function because it needs to run the same
code= path twice (pg_ctl and pg_resetwal).

+int
+main(int a= rgc, char **argv)

Sort the options alphabet= ically (long_options struct, getopt_long()
argument, switc= h cases).

Done.
=
- .../t/04= 0_pg_createsubscriber.pl
- .../t/041_pg_createsubscriber_s= tandby.pl

These two files could be combined= into one.

Done.
+# Force it t= o initialize a new cluster instead of copying a
+# previou= sly initdb'd cluster.

Explain why?

Ok. It needs a new cluster because it = will have a different system identifier
so we can make sur= e the target cluster is a copy of the source server.

<= /div>
+$node_s->ap= pend_conf(
+ 'postgresql.conf', qq[
+log_min= _messages =3D debug2

Is this setting necess= ary for the test?

No. It is he= re as a debugging aid. Better to include it in a separate patch.
There are a few messages that I don't intend to include in the fi= nal patch.

All of these modifications will = be included in the next patch. I'm finishing to
integrate = patches proposed by Hayato [1] and some additional fixes and
refactors.




--
Euler Taveira
EDB&n= bsp;  https://www.enterpr= isedb.com/

--2a3bece6ef59455b84179886a6ceac61--