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 1rVKaB-00GGcg-F0 for pgsql-hackers@arkaria.postgresql.org; Thu, 01 Feb 2024 00:05:32 +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 1rVKa8-00HSBh-2p for pgsql-hackers@arkaria.postgresql.org; Thu, 01 Feb 2024 00:05:28 +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 1rVKa7-00HSBZ-2h for pgsql-hackers@lists.postgresql.org; Thu, 01 Feb 2024 00:05:27 +0000 Received: from wout4-smtp.messagingengine.com ([64.147.123.20]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rVKa2-004PK3-0a for pgsql-hackers@lists.postgresql.org; Thu, 01 Feb 2024 00:05:25 +0000 Received: from compute2.internal (compute2.nyi.internal [10.202.2.46]) by mailout.west.internal (Postfix) with ESMTP id BCE923200B64; Wed, 31 Jan 2024 19:05:17 -0500 (EST) Received: from imap49 ([10.202.2.99]) by compute2.internal (MEProxy); Wed, 31 Jan 2024 19:05:18 -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=1706745917; x=1706832317; bh=7HOsM6oLb2 01bSDwBbmP2cnITjqSuce8RdtIHblcTmI=; b=Q01YbDOvVt3LZrFuWI/R6dAWt/ 2mXxm53Lj7t2QihSUp2pksFzbANCXQC7Rhbe3gQvT/47qJozUdoUMvU7ddRtp57t 3ucyGW3Ee+hEWb+zl8oilAjf57Oj1lBhxfsrdTaa9rEzrKLD3UffyTYfdLnHWMIo ViMBq6GoDBLQJYYf5v4o/C74g5mDo4GU3Y4L+HJyjoPlJ20SaLcBQKrVSjqrK808 n13IgRhvJmifvfos9Y5CG9wccYc5ycLY2/ZgZLDhSPgOa9V45azDVO1MAHdwunF1 wbRwZ+eORUowwlRsg2KC9dbo8IRvpUIg35PtOxpDQB87zjxzMoUNj65GWpIw== 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=1706745917; x=1706832317; bh=7HOsM6oLb201bSDwBbmP2cnITjqS uce8RdtIHblcTmI=; b=kcbPkqlnZzp2vr4WdTYMt0MYhZI3LoriPARS3LkJGf0Q 4N7ct4N5bkQvo+1KRlGol7sGP4KSgPTCjP8Cvv6g/dWlqczI8zcVaYVrPnD75ZZV iTMecosjIfXG47yTzWQ19+e0nBG58mPzZL/L9wvRMlZQ8pldWoxzjK94thMHAkue 89sVdZpTP/9zejD66yEPZCYLMi/SW9GI+hsXsienFeBy6EO8IGzibXfz8Pqn//f8 C4qRSKI4Bt2oW4CffMULzXJR8HaFsom1FPo2dIpNopJeSErm0w3GcxvkhjN2r1Hi m+uvJuMYMtEcerEFntdWOcUuiKZPHMXtAWktwnYa9Q== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvkedrfedutddgudekucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepofgfggfkjghffffhvfevufgtsegrtderreerredtnecuhfhrohhmpedfgfhu lhgvrhcuvfgrvhgvihhrrgdfuceovghulhgvrhesvghulhgvrhhtohdrtghomheqnecugg ftrfgrthhtvghrnhephedvveeuleejkeefleeihfeffefgffegueefhfdvueegtddtgedv heeuvdfgudfgnecuffhomhgrihhnpehpohhsthhgrhgvshhqlhdrohhrghdpvghnthgvrh hprhhishgvuggsrdgtohhmnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehm rghilhhfrhhomhepvghulhgvrhesvghulhgvrhhtohdrtghomh X-ME-Proxy: Feedback-ID: i0c21471d:Fastmail Received: by mailuser.nyi.internal (Postfix, from userid 501) id 6E28615A0094; Wed, 31 Jan 2024 19:05:16 -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: <80ce3f65-7ca3-471e-b1a4-24ac529ff4ea@app.fastmail.com> 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> <1cd45c1e-8ed1-42fc-9357-f1c43e0d6ac8@app.fastmail.com> Date: Wed, 31 Jan 2024 21:04:55 -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=05e803a7e78c4a5eb03b1e3f4089cb6e List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --05e803a7e78c4a5eb03b1e3f4089cb6e Content-Type: text/plain On Tue, Jan 30, 2024, at 6:26 AM, Hayato Kuroda (Fujitsu) wrote: > > One open item that is worrying me is how to handle the pg_ctl timeout. This > > patch does nothing and the user should use PGCTLTIMEOUT environment variable to > > avoid that the execution is canceled after 60 seconds (default for pg_ctl). > > Even if you set a high value, it might not be enough for cases like > > time-delayed replica. Maybe pg_ctl should accept no timeout as --timeout > > option. I'll include this caveat into the documentation but I'm afraid it is > > not sufficient and we should provide a better way to handle this situation. > > I felt you might be confused a bit. Even if the recovery_min_apply_delay is set, > e.g., 10h., the pg_ctl can start and stop the server. This is because the > walreceiver serialize changes as soon as they received. The delay is done by the > startup process. There are no unflushed data, so server instance can be turned off. > If you meant the combination of recovery-timeout and time-delayed replica, yes, > it would be likely to occur. But in the case, using like --no-timeout option is > dangerous. I think we should overwrite recovery_min_apply_delay to zero. Thought? I didn't provide the whole explanation. I'm envisioning the use case that pg_ctl doesn't reach the consistent state and the timeout is reached (the consequence is that pg_createsubscriber aborts the execution). It might occur on a busy server. The probability that it occurs with the current code is low (LSN gap for recovery is small). Maybe I'm anticipating issues when the base backup support is added but better to raise concerns during development. > Below part contains my comments for v11-0001. Note that the ordering is random. Hayato, thanks for reviewing v11. > 01. doc > ``` > > > > > ``` > > According to other documentation like pg_upgrade, we do not write both longer > and shorter options in the synopsis section. pg_upgrade doesn't but others do like pg_rewind, pg_resetwal, pg_controldata, pg_checksums. It seems newer tools tend to provide short and long options. > 02. doc > ``` > > pg_createsubscriber takes the publisher and subscriber > connection strings, a cluster directory from a physical replica and a list of > database names and it sets up a new logical replica using the physical > recovery process. > > > ``` > > I found that you did not include my suggestion without saying [1]. Do you dislike > the comment or still considering? Documentation is on my list. I didn't fix the documentation since some design decisions were changed. I'm still working on it. > 03. doc > ``` > > ``` > > Too many blank after -P. Fixed. [documentation related items will be addressed later...] > > 07. general > I think there are some commenting conversions in PG, but this file breaks it. It is on my list. > 08. general > Some pg_log_error() + exit(1) can be replaced with pg_fatal(). Done. I kept a few pg_log_error() + exit() because there is no pg_fatal_and_hint() function. > > 09. LogicalRepInfo > ``` > char *subconninfo; /* subscription connection string for logical > * replication */ > ``` > > As I posted in comment#8[2], I don't think it is "subscription connection". Also, > "for logical replication" is bit misreading because it would not be passed to > workers. Done. s/publication/publisher/ s/subscription/subscriber/ > 10. get_base_conninfo > ``` > static char * > get_base_conninfo(char *conninfo, char *dbname, const char *noderole) > ... > /* > * If --database option is not provided, try to obtain the dbname from > * the publisher conninfo. If dbname parameter is not available, error > * out. > */ > > ``` > > I'm not sure getting dbname from the conninfo improves user-experience. I felt > it may trigger an unintended targeting. > (I still think the publisher-server should be removed) Why not? Unique database is a common setup. It is unintended if you don't document it accordingly. I'll make sure it is advertised in the --database and the --publisher-server options. > 11. check_data_directory > ``` > /* > * Is it a cluster directory? These are preliminary checks. It is far from > * making an accurate check. If it is not a clone from the publisher, it will > * eventually fail in a future step. > */ > static bool > check_data_directory(const char *datadir) > ``` > > We shoud also check whether pg_createsubscriber can create a file and a directory. > GetDataDirectoryCreatePerm() verifies it. Good point. It is included in the next patch. > 12. main > ``` > /* Register a function to clean up objects in case of failure. */ > atexit(cleanup_objects_atexit); > ``` > > According to the manpage, callback functions would not be called when it exits > due to signals: > > > Functions registered using atexit() (and on_exit(3)) are not called if a > > process terminates abnormally because of the delivery of a signal. > > Do you have a good way to handle the case? One solution is to output created > objects in any log level, but the consideration may be too much. Thought? Nothing? If you interrupt the execution, there will be objects left behind and you, as someone that decided to do it, have to clean things up. What do you expect this tool to do? The documentation will provide some guidance informing the object name patterns this tool uses and you can check for leftover objects. Someone can argue that is a valid feature request but IMO it is not one in the top of the list. > 13, main > ``` > /* > * Create a temporary logical replication slot to get a consistent LSN. > ``` > > Just to clarify - I still think the process exits before here in case of dry run. > In case of pg_resetwal, the process exits before doing actual works like > RewriteControlFile(). Why? Are you suggesting that the dry run mode covers just the verification part? If so, it is not a dry run mode. I would expect it to run until the end (or until it accomplish its goal) but *does not* modify data. For pg_resetwal, the modification is one of the last steps and the other ones (KillFoo functions) that are skipped modify data. It ends the dry run mode when it accomplish its goal (obtain the new control data values). If we stop earlier, some of the additional steps won't be covered by the dry run mode and a failure can happen but could be detected if you run a few more steps. > 14. main > ``` > * XXX we might not fail here. Instead, we provide a warning so the user > * eventually drops this replication slot later. > ``` > > But there are possibilities to exit(1) in drop_replication_slot(). Is it acceptable? No, there isn't. > 15. wait_for_end_recovery > ``` > /* > * Bail out after recovery_timeout seconds if this option is set. > */ > if (recovery_timeout > 0 && timer >= recovery_timeout) > ``` > > Hmm, IIUC, it should be enabled by default [3]. Do you have anything in your mind? Why? See [1]. I prefer the kind mode (always wait until the recovery ends) but you and Amit are proposing a more aggressive mode. The proposal (-t 60) seems ok right now, however, if the goal is to provide base backup support in the future, you certainly should have to add the --recovery-timeout in big clusters or those with high workloads because base backup is run between replication slot creation and consistent LSN. Of course, we can change the default when base backup support is added. > 16. main > ``` > /* > * Create the subscription for each database on subscriber. It does not > * enable it immediately because it needs to adjust the logical > * replication start point to the LSN reported by consistent_lsn (see > * set_replication_progress). It also cleans up publications created by > * this tool and replication to the standby. > */ > if (!setup_subscriber(dbinfo, consistent_lsn)) > ``` > > Subscriptions would be created and replication origin would be moved forward here, > but latter one can be done only by the superuser. I felt that this should be > checked in check_subscriber(). Good point. I included a check for pg_create_subscription role and CREATE privilege on the specified database. > 17. main > ``` > /* > * Change system identifier. > */ > modify_sysid(pg_resetwal_path, subscriber_dir); > ``` > > Even if I executed without -v option, an output from pg_resetwal command appears. > It seems bit strange. The pg_resetwal is using a printf and there is no prefix that identifies that message is from pg_resetwal. That's message has been bothering me for a while so let's send it to /dev/null. I'll include it in the next patch. RewriteControlFile(); KillExistingXLOG(); KillExistingArchiveStatus(); KillExistingWALSummaries(); WriteEmptyXLOG(); printf(_("Write-ahead log reset\n")); return 0; [1] https://www.postgresql.org/message-id/b315c7da-7ab1-4014-a2a9-8ab6ae26017c%40app.fastmail.com -- Euler Taveira EDB https://www.enterprisedb.com/ --05e803a7e78c4a5eb03b1e3f4089cb6e Content-Type: text/html Content-Transfer-Encoding: quoted-printable
On Tue, Ja= n 30, 2024, at 6:26 AM, Hayato Kuroda (Fujitsu) wrote:
> One open item that is w= orrying me is how to handle the pg_ctl timeout. This
> = patch does nothing and the user should use PGCTLTIMEOUT environment vari= able to
> avoid that the execution is canceled after 60= seconds (default for pg_ctl).
> Even if you set a high= value, it might not be enough for cases like
> time-de= layed replica. Maybe pg_ctl should accept no timeout as --timeout
> option. I'll include this caveat into the documentation but= I'm afraid it is
> not sufficient and we should provid= e a better way to handle this situation.

I = felt you might be confused a bit. Even if the recovery_min_apply_delay i= s set,
e.g., 10h., the pg_ctl can start and stop the serve= r. This is because the
walreceiver serialize changes as so= on as they received. The delay is done by the
startup proc= ess. There are no unflushed data, so server instance can be turned off.<= br>
If you meant the combination of recovery-timeout and time-= delayed replica, yes,
it would be likely to occur. But in = the case, using like --no-timeout option is
dangerous. I t= hink we should overwrite recovery_min_apply_delay to zero. Thought?
<= /div>

I didn't provide the whole explana= tion. I'm envisioning the use case that pg_ctl
doesn't rea= ch the consistent state and the timeout is reached (the consequence
<= /div>
is that pg_createsubscriber aborts the execution). It might oc= cur on a busy
server. The probability that it occurs with = the current code is low (LSN gap
for recovery is small). M= aybe I'm anticipating issues when the base backup
support = is added but better to raise concerns during development.
=
Below part= contains my comments for v11-0001. Note that the ordering is random.

Hayato, thanks for reviewing v11= .

01. doc
```
    <gro= up choice=3D"req">
     <arg cho= ice=3D"plain"><option>-D</option> </arg>
<= div>     <arg choice=3D"plain"><option>-= -pgdata</option></arg>
    <= /group>
```

According to o= ther documentation like pg_upgrade, we do not write both longer
and shorter options in the synopsis section.

pg_upgrade doesn't but others do like pg_rewind, pg= _resetwal, pg_controldata,
pg_checksums. It seems newer to= ols tend to provide short and long options.

02. doc
``= `
  <para>
   <appli= cation>pg_createsubscriber</application> takes the publisher an= d subscriber
   connection strings, a cluster di= rectory from a physical replica and a list of
  = database names and it sets up a new logical replica using the physical<= br>
   recovery process.
  </p= ara>

```

I = found that you did not include my suggestion without saying [1]. Do you = dislike
the comment or still considering?

Documentation is on my list. I didn't fix the = documentation since some design
decisions were changed. I'= m still working on it.

03. doc
```
 = ;     <term><option>-P  <replace= able class=3D"parameter">connstr</replaceable></option>&l= t;/term>
```

Too many blan= k after -P.

Fixed.

[documentation related items will be addressed later..= .]


07. general
I think there are some comme= nting conversions in PG, but this file breaks it.
=

It is on my list.

08. general
Some= pg_log_error() + exit(1) can be replaced with pg_fatal().

Done. I kept a few pg_log_error() + exit() = because there is no
pg_fatal_and_hint() function.


=
09. LogicalRepInfo
```
char  = ;  *subconninfo; /* subscription connection string for logical
<= /div>
* replication */
```

As I posted in comment#8[2], I don't think it is "subscription connect= ion". Also,
"for logical replication" is bit misreading be= cause it would not be passed to
workers.

Done.

s/publicatio= n/publisher/
s/subscription/subscriber/

=
10. get_base_c= onninfo
```
static char *
get_= base_conninfo(char *conninfo, char *dbname, const char *noderole)
...
/*
* If --database option is not= provided, try to obtain the dbname from
* the publisher c= onninfo. If dbname parameter is not available, error
* out= .
*/

```

I'm not sure getting dbname from the conninfo improves user-exp= erience. I felt
it may trigger an unintended targeting.
(I still think the publisher-server should be removed)

Why not? Unique database is a commo= n setup. It is unintended if you don't
document it accordi= ngly. I'll make sure it is advertised in the --database and
the --publisher-server options.

11. check_data_directory
=
```
/*
* Is it a cluster directory? The= se are preliminary checks. It is far from
* making an accu= rate check. If it is not a clone from the publisher, it will
* eventually fail in a future step.
*/
st= atic bool
check_data_directory(const char *datadir)
```

We shoud also check whether pg_= createsubscriber can create a file and a directory.
GetDat= aDirectoryCreatePerm() verifies it.

Good point. It is included in the next patch.

12. main
```
/* Register a function to clean up objects in c= ase of failure. */
atexit(cleanup_objects_atexit);
```

According to the manpage, callba= ck functions would not be called when it exits
due to sign= als:

> Functions  registered  = using atexit() (and on_exit(3)) are not called if a
> p= rocess terminates abnormally because of the delivery of a signal.

Do you have a good way to handle the case? One so= lution is to output created
objects in any log level, but = the consideration may be too much. Thought?
<= br>
Nothing? If you interrupt the execution, there will be obj= ects left behind and
you, as someone that decided to do it= , have to clean things up. What do you
expect this tool to= do? The documentation will provide some guidance informing
the object name patterns this tool uses and you can check for leftover= objects.
Someone can argue that is a valid feature reques= t but IMO it is not one in the
top of the list.
<= div>
13, ma= in
```
/*
* Create a temporary= logical replication slot to get a consistent LSN.
```
=

Just to clarify - I still think the process ex= its before here in case of dry run.
In case of pg_resetwal= , the process exits before doing actual works like
Rewrite= ControlFile().

Why? Are you su= ggesting that the dry run mode covers just the verification
part? If so, it is not a dry run mode. I would expect it to run until = the end
(or until it accomplish its goal) but *does not* m= odify data. For pg_resetwal,
the modification is one of th= e last steps and the other ones (KillFoo
functions) that a= re skipped modify data. It ends the dry run mode when it
a= ccomplish its goal (obtain the new control data values). If we stop earl= ier,
some of the additional steps won't be covered by the = dry run mode and a failure
can happen but could be detecte= d if you run a few more steps.

14. main
```
* XXX we might not fail here. Instead, we provide a warning so the use= r
* eventually drops this replication slot later.
```

But there are possibilities to ex= it(1) in drop_replication_slot(). Is it acceptable?

No, there isn't.

15. wait_for_end_recovery
=
```
/*
* Bail out after recovery_= timeout seconds if this option is set.
*/
if= (recovery_timeout > 0 && timer >=3D recovery_timeout)
=
```

Hmm, IIUC, it should be enab= led by default [3]. Do you have anything in your mind?

Why? See [1]. I prefer the kind mode (always wa= it until the recovery ends) but
you and Amit are proposing= a more aggressive mode. The proposal (-t 60) seems
ok rig= ht now, however, if the goal is to provide base backup support in the
future, you certainly should have to add the --recovery-time= out in big clusters
or those with high workloads because b= ase backup is run between replication slot
creation and co= nsistent LSN. Of course, we can change the default when base
backup support is added.

16. main
```
/*
* Create the subscription for each database on subscri= ber. It does not
* enable it immediately because it needs = to adjust the logical
* replication start point to the LSN= reported by consistent_lsn (see
* set_replication_progres= s). It also cleans up publications created by
* this tool = and replication to the standby.
*/
if (!setu= p_subscriber(dbinfo, consistent_lsn))
```
Subscriptions would be created and replication origin would= be moved forward here,
but latter one can be done only by= the superuser. I felt that this should be
checked in chec= k_subscriber().

Good point. I = included a check for pg_create_subscription role and CREATE
privilege on the specified database.

17. main
```
/*
* Change system identifier.
*= /
modify_sysid(pg_resetwal_path, subscriber_dir);
```

Even if I executed without -v opt= ion, an output from pg_resetwal command appears.
It seems = bit strange.

The pg_resetwal i= s using a printf and there is no prefix that identifies that
message is from pg_resetwal. That's message has been bothering me for= a while
so let's send it to /dev/null. I'll include it in= the next patch.

    Rewrite= ControlFile();
    KillExistingXLOG();
<= /div>
    KillExistingArchiveStatus();
=     KillExistingWALSummaries();
  = ;  WriteEmptyXLOG();

   = ; printf(_("Write-ahead log reset\n"));
   = return 0;



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

--05e803a7e78c4a5eb03b1e3f4089cb6e--