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 1rnAMk-00BuSs-Bz for pgsql-hackers@arkaria.postgresql.org; Thu, 21 Mar 2024 04:49:23 +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 1rnAMg-00A6aT-Uo for pgsql-hackers@arkaria.postgresql.org; Thu, 21 Mar 2024 04:49:19 +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 1rnAMg-00A6aK-CN for pgsql-hackers@lists.postgresql.org; Thu, 21 Mar 2024 04:49:19 +0000 Received: from fout3-smtp.messagingengine.com ([103.168.172.146]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rnAMb-005bie-Ga for pgsql-hackers@lists.postgresql.org; Thu, 21 Mar 2024 04:49:17 +0000 Received: from compute2.internal (compute2.nyi.internal [10.202.2.46]) by mailfout.nyi.internal (Postfix) with ESMTP id 1213713800D7; Thu, 21 Mar 2024 00:49:14 -0400 (EDT) Received: from imap49 ([10.202.2.99]) by compute2.internal (MEProxy); Thu, 21 Mar 2024 00:49:14 -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=fm3; t=1710996554; x=1711082954; bh=F+lIbuyo66 zJHVyhBV+YMc4wgatHUu8LPqmfXBJl+gc=; b=GRHM5N3egL5Ui7JrGTKKHSjN2C 6CBQmrqbqyLMvgI5rnEwqQWd0KOOucc8lCXz7Qs8ViTjWxpCCTwt620dTD6eLJWU VmBBWJ2qlYuftAQACLhbiejaQDUs6JtSix/4Bv/bj/1ffvQfFh+TLZyKpwl8A7SS wQledgBT6mcmQdwavv5J6izzVdkln9tgF2gSLNyVzFwbXrhheL2jN9+9Lwh7BWXs qAF2hRZEPUDcllEEMSKquHNRT7QlL/3EKrVopXiBXqFSngSraVZ+wBMIDAt8JH/e Zv6xtNF5qIsYPp/ad1JWTazEoegsO5pzyiKbQ8yM1ubUBL/gTq1KZG+Cf0/g== 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= fm2; t=1710996554; x=1711082954; bh=F+lIbuyo66zJHVyhBV+YMc4wgatH Uu8LPqmfXBJl+gc=; b=QBWhHkEooWUIb7eFwe0HavN04Ck+uotJLD91vv22/b05 YG2+JlJmzreg1+txLH0b1iU9OKTEWc7rlF3bs4AU4JbVcs/baRwrv/tVoQ7bR0qM 99ZYl1RqwPsVU28SXLXY72gxXMCOcJBFhP+2ep1rYl1/pANM5I+GPyKcNLJteTg6 v7nB2tAaoFd4JcaQjmJVt/OK54l+r/AZS6HHyM98mbxrZ1AiIuwfxjNJwGm+M2y7 foJ3GSWtR/INEYeZx/qycAVaOixULojWCD/igckfDvdvvS3lA8OhNUBijwX/0hbu 2BoUz3dAdGI5M7GJ7x6oiqznse85KcUg2ILCGOCcQg== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvledrleehgdejgecutefuodetggdotefrodftvf 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 620D815A0092; Thu, 21 Mar 2024 00:49:13 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.11.0-alpha0-332-gdeb4194079-fm-20240319.002-gdeb41940 MIME-Version: 1.0 Message-Id: In-Reply-To: References: <9a9c7e65-c47b-4400-a63e-ac6169c0860e@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> <40595e73-c7e1-463a-b8be-49792e870007@app.fastmail.com> <6423dfeb-a729-45d3-b71e-7bf1b3adb0c9@enterprisedb.com> <2f0b73f3-e7c0-4e95-9712-1993442a6b64@app.fastmail.com> <34637e7f-0330-420d-8f45-1d022962d2fe@app.fastmail.com> Date: Thu, 21 Mar 2024 01:48:30 -0300 From: "Euler Taveira" To: "kuroda.hayato@fujitsu.com" , "Amit Kapila" Cc: "Shlok Kyal" , "Tomas Vondra" , "pgsql-hackers@lists.postgresql.org" , "Michael Paquier" , "Peter Eisentraut" , "Andres Freund" , "Ashutosh Bapat" , =?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= , "vignesh C" Subject: Re: speed up a logical replica setup Content-Type: multipart/alternative; boundary=5a2267098f4a40bbb396ccd92fce787a List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --5a2267098f4a40bbb396ccd92fce787a Content-Type: text/plain On Mon, Mar 18, 2024, at 2:43 AM, Hayato Kuroda (Fujitsu) wrote: > Thanks for updating the patch. Here are my comments. > I used Grammarly to proofread sentences. > (The tool strongly recommends to use active voice, but I can ignore for now) Thanks for another review. I posted a new patch (v32) that hopefully addresses these points. > 01. > > "After a successful run, the state of the target server is analagous to a fresh > logical replication setup." > a/analagous/analogous Fixed. > 02. > > "The main difference between the logical replication setup and pg_createsubscriber > is the initial data copy." > > Grammarly suggests: > "The initial data copy is the main difference between the logical replication > setup and pg_createsubscriber." Not fixed. > 03. > > "Only the synchronization phase is done, which ensures each table is brought up > to a synchronized state." > > This sentence is not very clear to me. How about: > "pg_createsubscriber does only the synchronization phase, ensuring each table's > replication state is ready." I avoided pg_createsubscriber at the beginning because it is already used in the previous sentence. I kept the last part of the sentence because it is similar to one in the logical replication [1]. > 04. > > "The pg_createsubscriber targets large database systems because most of the > execution time is spent making the initial data copy." > > Hmm, but initial data sync by logical replication also spends most of time to > make the initial data copy. IIUC bottlenecks are a) this application must stop > and start server several times, and b) only the serial copy works. Can you > clarify them? Reading the sentence again, it is not clear. When I said "most of execution time" I was referring to the actual logical replication setup. > 05. > > It is better to say the internal difference between pg_createsubscriber and the > initial sync by logical replication. For example: > pg_createsubscriber uses a physical replication mechanism to ensure the standby > catches up until a certain point. Then, it converts to the standby to the > subscriber by promoting and creating subscriptions. Isn't it better to leave these details to "How It Works"? > 06. > > "If these are not met an error will be reported." > > Grammarly suggests: > "If these are not met, an error will be reported." Fixed. > 07. > > "The given target data directory must have the same system identifier than the > source data directory." > > Grammarly suggests: > "The given target data directory must have the same system identifier as the > source data directory." Fixed. > 08. > > "If a standby server is running on the target data directory or it is a base > backup from the source data directory, system identifiers are the same." > > This line is not needed if bullet-style is not used. The line is just a supplement, > not prerequisite. Fixed. > 09. > > "The source server must accept connections from the target server. The source server must not be in recovery." > > Grammarly suggests: > "The source server must accept connections from the target server and not be in recovery." Not fixed. > 10. > > "Publications cannot be created in a read-only cluster." > > Same as 08, this line is not needed if bullet-style is not used. Fixed. > 11. > > "pg_createsubscriber usually starts the target server with different connection > settings during the transformation steps. Hence, connections to target server > might fail." > > Grammarly suggests: > "pg_createsubscriber usually starts the target server with different connection > settings during transformation. Hence, connections to the target server might fail." Fixed. > 12. > > "During the recovery process," > > Grammarly suggests: > "During recovery," Not fixed. Our documentation uses "recovery process". > 13. > > "replicated so an error would occur." > > Grammarly suggests: > "replicated, so an error would occur." I didn't find this one. Maybe you checked a previous version. > 14. > > "It would avoid situations in which WAL files from the source server might be > used by the target server." > > Grammarly suggests: > "It would avoid situations in which the target server might use WAL files from > the source server." Fixed. > 15. > > "a LSN" > > s/a/an Fixed. > 16. > > "of write-ahead" > > s/of/of the/ Fixed. > 17. > > "specifies promote" > > We can do double-quote for the word promote. Why? It is referring to recovery_target_action. If you check this GUC, you will notice that it also uses literal tag. > 18. > > "are also added so it avoids" > > Grammarly suggests: > "are added to avoid" Fixed. > 19. > > "is accepting read-write transactions" > > Grammarly suggests: > "accepts read-write transactions" Not fixed. > 20. > > New options must be also documented as well. This helps not only users but also > reviewers. > (Sometimes we cannot identify that the implementation is intentinal or not.) I don't know what are you referring to? If the new options are --publication, --subscription and --replication-slot, they are documented. Are you checking the latest patch? > 21. > > Also, not sure the specification is good. I preferred to specify them by format > string. Because it can reduce the number of arguments and I cannot find use cases > which user want to control the name of objects. > > However, your approach has a benefit which users can easily identify the generated > objects by pg_createsubscriber. How do other think? I prefer explicit options. We can always expand it later if people think it is a good idea to provide a format string. > 22. > > ``` > #define BASE_OUTPUT_DIR "pg_createsubscriber_output.d" > ``` > > No one refers the define. It was removed in v30. > 23. > > ``` > } CreateSubscriberOptions; > ... > } LogicalRepInfo; > ``` > > Declarations after the "{" are not needed, because we do not do typedef. It is a leftover when I removed the typedef. > 22. > > While seeing definitions of functions, I found that some pointers are declared > as const, but others are not. E.g., "char *lsn" in setup_recovery() won' be > changed but not the constant. Is it just missing or is there another rule? It slipped my mind. Peter's fixups improves it. > 23. > > ``` > static int num_dbs = 0; > static int num_pubs = 0; > static int num_subs = 0; > static int num_replslots = 0; > ``` > > I think the name is bit confusing. The number of generating publications/subscriptions/replication slots > are always same as the number of databases. They just indicate the number of > specified. > > My idea is num_custom_pubs or something. Thought? What does "custom" add to make the name clear? I added comments saying so. > 24. > > ``` > /* standby / subscriber data directory */ > static char *subscriber_dir = NULL; > ``` > > It is bit strange that only subscriber_dir is a global variable. Caller requires > the CreateSubscriberOptions as an argument, except cleanup_objects_atexit() and > main. So, how about makeing `CreateSubscriberOptions opt` to global one? I avoided turning all the options global variables. Since the cleanup routine required the target data directory to be a global variable, I just did it and left the others alone. > 25. > > ``` > * Replication slots, publications and subscriptions are created. Depending on > * the step it failed, it should remove the already created objects if it is > * possible (sometimes it won't work due to a connection issue). > ``` > > I think it should be specified here that subscriptions won't be removed with the > reason. I rephrased this comment. > 26. > > ``` > > /* > * If the server is promoted, there is no way to use the current setup > * again. Warn the user that a new replication setup should be done before > * trying again. > */ > ``` > > Per comment 25, we can add a reference like "See comments atop the function" It is a few lines above. I don't think you have to point it out. If you are unsure about this decision, you should check the whole function. > 27. > > usage() was not updated based on recent changes. Check v30. > 28. > > ``` > if (strcmp(conn_opt->keyword, "dbname") == 0 && conn_opt->val != NULL) > { > if (dbname) > *dbname = pg_strdup(conn_opt->val); > continue; > } > ``` > > There is a memory-leak if multiple dbname are specified in the conninfo. It is not a worrying or critical memory leak. > 29. > > ``` > pg_prng_seed(&prng_state, (uint64) (getpid() ^ time(NULL))); > ``` > > No need to initialize the seed every time. Can you reuse pg_prng_state? Sure. > 30. > > ``` > if (num_replslots == 0) > dbinfo[i].replslotname = pg_strdup(genname); > ``` > > I think the straightforward way is to use the name of subscription if no name > is specified. This follows the rule for CREATE SUBSCRIPTION. Agreed. > 31. > > ``` > /* Create replication slot on publisher */ > if (lsn) > pg_free(lsn); > ``` > > I think allocating/freeing memory is not so efficient. > Can we add a flag to create_logical_replication_slot() for controlling the > returning value (NULL or duplicated string)? We can use the condition (i == num_dbs-1) > as flag. It is not. This code path is not critical. You are suggesting to add complexity here. Efficiency is a good goal but in this case it only adds complexity with small return. > 32. > > ``` > /* > * Close the connection. If exit_on_error is true, it has an undesired > * condition and it should exit immediately. > */ > static void > disconnect_database(PGconn *conn, bool exit_on_error) > ``` > > In case of disconnect_database(), the second argument should have different name. > If it is true, the process exits unconditionally. > Also, comments atop the function must be fixed. I choose a short name. The comment seems ok to me. > > 33. > > ``` > wal_level = strdup(PQgetvalue(res, 0, 0)); > ``` > > pg_strdup should be used here. Fixed. > 34. > > ``` > {"config-file", required_argument, NULL, 1}, > {"publication", required_argument, NULL, 2}, > {"replication-slot", required_argument, NULL, 3}, > {"subscription", required_argument, NULL, 4}, > ``` > > The ordering looks strange for me. According to pg_upgarade and pg_basebackup, > options which do not have short notation are listed behind. Fixed. > 35. > > ``` > opt.sub_port = palloc(16); > ``` > > Per other lines, pg_alloc() should be used. I think you meant pg_malloc. Fixed. > 36. > > ``` > pg_free(opt.sub_port); > ``` > > You said that the leak won't be concerned here. If so, why only 'p' has pg_free()? Fixed. > 37. > > ``` > /* Register a function to clean up objects in case of failure */ > atexit(cleanup_objects_atexit); > ``` > > Sorry if we have already discussed. I think the registration can be moved just > before the boot of the standby. Before that, the callback will be no-op. The main reason is to catch future cases added *before* the point you want to move this call that requires a cleanup. As you said it is a no-op. My preference for atexit() calls is to add it as earlier as possible to avoid leaving cases that it should trigger. > 38. > > ``` > /* Subscriber PID file */ > snprintf(pidfile, MAXPGPATH, "%s/postmaster.pid", subscriber_dir); > > /* > * If the standby server is running, stop it. Some parameters (that can > * only be set at server start) are informed by command-line options. > */ > if (stat(pidfile, &statbuf) == 0) > ``` > > Hmm. pidfile is used only here, but it is declared in main(). Can it be > separated into another funtion like is_standby_started()? It is so small that I didn't bother adding a new function for it. > 39. > > Or, we may able to introcue "restart_standby_if_needed" or something. > > 40. > > ``` > * XXX this code was extracted from BootStrapXLOG(). > ``` > > So, can we extract the common part to somewhere? Since system identifier is related > with the controldata file, I think it can be located in controldata_util.c. I added this comment here as a reference from where I extracted the code. The referred function is from backend. Feel free to propose a separate patch for it. > 41. > > You said like below in [1], but I could not find the related fix. Can you clarify? > > > That's a good point. We should state in the documentation that GUCs specified in > > the command-line options are ignored during the execution. I added a sentence for it. See "How It Works". [1] https://www.postgresql.org/docs/current/logical-replication-architecture.html#LOGICAL-REPLICATION-SNAPSHOT -- Euler Taveira EDB https://www.enterprisedb.com/ --5a2267098f4a40bbb396ccd92fce787a Content-Type: text/html Content-Transfer-Encoding: quoted-printable
On Mon, Ma= r 18, 2024, at 2:43 AM, Hayato Kuroda (Fujitsu) wrote:
Thanks for updating the patc= h. Here are my comments.
I used Grammarly to proofread sen= tences.
(The tool strongly recommends to use active voice,= but I can ignore for now)

Tha= nks for another review. I posted a new patch (v32) that hopefully addres= ses
these points.

01.

"Af= ter a successful run, the state of the target server is analagous to a f= resh
logical replication setup."
a/analagous= /analogous

Fixed.

02.
<= /div>

"The main difference between the logical replic= ation setup and pg_createsubscriber
is the initial data co= py."

Grammarly suggests:
"The= initial data copy is the main difference between the logical replicatio= n
setup and pg_createsubscriber."

Not fixed.

03.

"Only = the synchronization phase is done, which ensures each table is brought u= p
to a synchronized state."

T= his sentence is not very clear to me. How about:
"pg_creat= esubscriber does only the synchronization phase, ensuring each table's
replication state is ready."
I avoided pg_createsubscriber at the beginning because it is= already
used in the previous sentence. I kept the last pa= rt of the sentence
because it is similar to one in the log= ical replication [1].

04.

"The pg_creat= esubscriber targets large database systems because most of the
=
execution time is spent making the initial data copy."

Hmm, but initial data sync by logical replication also = spends most of time to
make the initial data copy. IIUC bo= ttlenecks are a) this application must stop
and start serv= er several times, and b) only the serial copy works. Can you
clarify them?

Reading the s= entence again, it is not clear. When I said "most of
execu= tion time" I was referring to the actual logical replication setup.
<= /div>

= 05.

It is better to say the internal differ= ence between pg_createsubscriber and the
initial sync by l= ogical replication. For example:
pg_createsubscriber uses = a physical replication mechanism to ensure the standby
cat= ches up until a certain point. Then, it converts to the standby to the
subscriber by promoting and creating subscriptions.

Isn't it better to leave these detail= s to "How It Works"?

06.

"If these are = not met an error will be reported."

Grammar= ly suggests:
"If these are not met, an error will be repor= ted."

Fixed.
07.
=

"The given target data directory must have the same = system identifier than the
source data directory."

Grammarly suggests:
"The given targe= t data directory must have the same system identifier as the
source data directory."

Fix= ed.

08.

"If a standby server is running= on the target data directory or it is a base
backup from = the source data directory, system identifiers are the same."

This line is not needed if bullet-style is not used. T= he line is just a supplement,
not prerequisite.
<= /blockquote>

Fixed.

09.

<= div>"The source server must accept connections from the target server. T= he source server must not be in recovery."

= Grammarly suggests:
"The source server must accept connect= ions from the target server and not be in recovery."

Not fixed.

10.

"= Publications cannot be created in a read-only cluster."
Same as 08, this line is not needed if bullet-style is not = used.

Fixed.
11.
=

"pg_createsubscriber usually starts the target serve= r with different connection
settings during the transforma= tion steps. Hence, connections to target server
might fail= ."
 
Grammarly suggests:
= "pg_createsubscriber usually starts the target server with different con= nection
settings during transformation. Hence, connections= to the target server might fail."

=
Fixed.

12.

"During the recovery pr= ocess,"

Grammarly suggests:
"= During recovery,"

Not fixed. O= ur documentation uses "recovery process".

13.

"replicated so an error would occur."

Grammarly suggests:
"replicated, so an error would occur= ."

I didn't find this one. May= be you checked a previous version.

14.

= "It would avoid situations in which WAL files from the source server mig= ht be
used by the target server."

=
Grammarly suggests:
"It would avoid situations in whi= ch the target server might use WAL files from
the source s= erver."

Fixed.
<= br>
15.

"a LSN"

s/a/an

Fixed.

16.

"of write-ahead"

s/of/of the/

Fixed.

17.

"specifies promote"

We can do doub= le-quote for the word promote.

Why? It is referring to recovery_target_action. If you check this GUC,<= br>
you will notice that it also uses literal tag.

18.
=

"are also added so it avoids"
Grammarly suggests:
"are added to avoid"
<= /div>

Fixed.

19.

<= /div>
"is accepting read-write transactions"

Grammarly suggests:
"accepts read-write transactions= "

Not fixed.
20.
=

New options must be also documented as well. This he= lps not only users but also
reviewers.
(Some= times we cannot identify that the implementation is intentinal or not.)<= br>

I don't know what are you refe= rring to? If the new options are
--publication, --subscrip= tion and --replication-slot, they are
documented. Are you = checking the latest patch?

21.

Also, no= t sure the specification is good. I preferred to specify them by format<= br>
string. Because it can reduce the number of arguments and = I cannot find use cases
which user want to control the nam= e of objects.

However, your approach has a = benefit which users can easily identify the generated
obje= cts by pg_createsubscriber. How do other think?

I prefer explicit options. We can always expand it lat= er if people think it is a
good idea to provide a format s= tring.

22.

```
#define BA= SE_OUTPUT_DIR "pg_createsubscriber_output.d"
```

No one refers the define.

It was removed in v30.

23.
 
=
```
} CreateSubscriberOptions;
= ...
} LogicalRepInfo;
```
Declarations after the "{" are not needed, because we do no= t do typedef.

It is a leftover= when I removed the typedef.

22.

While = seeing definitions of functions, I found that some pointers are declared=
as const, but others are not. E.g., "char *lsn" in setup_= recovery() won' be
changed but not the constant. Is it jus= t missing or is there another rule?

It slipped my mind. Peter's fixups improves it.

=
23.
<= div>
```
static int num_dbs =3D 0;
=
static int num_pubs =3D 0;
static int num_subs =3D 0;=
static int num_replslots =3D 0;
```

I think the name is bit confusing. The number of g= enerating publications/subscriptions/replication slots
are= always same as the number of databases. They just indicate the number o= f
specified.

My idea is num_c= ustom_pubs or something. Thought?

<= div>What does "custom" add to make the name clear? I added comments sayi= ng
so.

24.

```
<= div>/* standby / subscriber data directory */
static char = *subscriber_dir =3D NULL;
```

It is bit strange that only subscriber_dir is a global variable. Caller= requires
the CreateSubscriberOptions as an argument, exce= pt cleanup_objects_atexit() and
main. So, how about makein= g `CreateSubscriberOptions opt` to global one?

I avoided turning all the options global variables. Sin= ce the cleanup routine
required the target data directory = to be a global variable, I just did it and
left the others= alone.

25.

```
* Replica= tion slots, publications and subscriptions are created. Depending on
=
* the step it failed, it should remove the already created ob= jects if it is
* possible (sometimes it won't work due to = a connection issue).
```

I th= ink it should be specified here that subscriptions won't be removed with= the
reason. 

I rephrased this comment.

26.

```

/*
* If the server is promoted,= there is no way to use the current setup
* again. Warn th= e user that a new replication setup should be done before
= * trying again.
*/
```

Per comment 25, we can add a reference like "See comments atop t= he function"

It is a few lines= above. I don't think you have to point it out. If you
are= unsure about this decision, you should check the whole function.

27= .

usage() was not updated based on recent c= hanges.

Check v30.

28.
=

```
if (strcmp(conn_opt->keyw= ord, "dbname") =3D=3D 0 && conn_opt->val !=3D NULL)
=
{
if (dbname)
*dbname =3D pg_strdup(con= n_opt->val);
continue;
}
``= `

There is a memory-leak if multiple dbname= are specified in the conninfo.

It is not a worrying or critical memory leak.

29.

```
pg_prng_seed(&prng_state, (uint64) (getpi= d() ^ time(NULL)));
```

No ne= ed to initialize the seed every time. Can you reuse pg_prng_state?

Sure.

30.

```
if (num_replslots =3D=3D 0)
dbin= fo[i].replslotname =3D pg_strdup(genname);
```

I think the straightforward way is to use the name of = subscription if no name
is specified. This follows the rul= e for CREATE SUBSCRIPTION.

Agr= eed.

31.

```
/* Create rep= lication slot on publisher */
if (lsn)
pg_fr= ee(lsn);
```

I think allocati= ng/freeing memory is not so efficient.
Can we add a flag t= o create_logical_replication_slot() for controlling the
re= turning value (NULL or duplicated string)? We can use the condition (i =3D= =3D num_dbs-1)
as flag.

It is not. This code path is not critical. You are suggesting to= add
complexity here. Efficiency is a good goal but in thi= s case it only adds
complexity with small return.

32.<= br>

```
/*
* Close = the connection. If exit_on_error is true, it has an undesired
<= div>* condition and it should exit immediately.
*/
static void
disconnect_database(PGconn *conn, bool = exit_on_error)
```

In case of= disconnect_database(), the second argument should have different name.<= br>
If it is true, the process exits unconditionally.
Also, comments atop the function must be fixed.

I choose a short name. The comment seems ok to m= e.


33.

```
wal_level =3D strdup(PQgetvalue(res, 0, 0));
```

pg_strdup should be used here.

Fixed.

34.

```
{"config-file", required_argument, NULL, 1},
{= "publication", required_argument, NULL, 2},
{"replication-= slot", required_argument, NULL, 3},
{"subscription", requi= red_argument, NULL, 4},
```

T= he ordering looks strange for me. According to pg_upgarade and pg_baseba= ckup,
options which do not have short notation are listed = behind.

Fixed.
<= br>
35.

```
opt.sub_port =3D palloc(16);
=
```

Per other lines, pg_alloc() = should be used.

I think you me= ant pg_malloc. Fixed.

36.

```
=
pg_free(opt.sub_port);
```

You said that the leak won't be concerned here. If so, why only 'p' h= as pg_free()?

Fixed.
=

37.

```
/* Register a function to = clean up objects in case of failure */
atexit(cleanup_obje= cts_atexit);
```

Sorry if we = have already discussed. I think the registration can be moved just
before the boot of the standby. Before that, the callback will = be no-op.

The main reason is t= o catch future cases added *before* the point you
want to = move this call that requires a cleanup. As you said it is a
no-op. My preference for atexit() calls is to add it as earlier as
=
possible to avoid leaving cases that it should trigger.

3= 8.

```
/* Subscriber PID file= */
snprintf(pidfile, MAXPGPATH, "%s/postmaster.pid", subs= criber_dir);

/*
* If the stan= dby server is running, stop it. Some parameters (that can
= * only be set at server start) are informed by command-line options.
=
*/
if (stat(pidfile, &statbuf) =3D=3D 0)
```

Hmm. pidfile is used only h= ere, but it is declared in main(). Can it be
separated int= o another funtion like is_standby_started()?
=
It is so small that I didn't bother adding a new function= for it.

39.

Or, we may able to introcue "re= start_standby_if_needed" or something.

40.<= br>

```
* XXX this code was extra= cted from BootStrapXLOG().
```

So, can we extract the common part to somewhere? Since system identifi= er is related
with the controldata file, I think it can be= located in controldata_util.c.

I added this comment here as a reference from where I extracted the
code. The referred function is from backend. Feel free to pr= opose a
separate patch for it.

41.

<= /div>
You said like below in [1], but I could not find the related f= ix. Can you clarify?

> That's a good poi= nt. We should state in the documentation that GUCs specified in
> the command-line options are ignored during the execution.

I added a sentence for it. See "= How It Works".




--
Euler Ta= veira
EDB   https://www.enterprisedb.com/

--5a2267098f4a40bbb396ccd92fce787a--