Received: from malur.postgresql.org ([217.196.149.56])
	by arkaria.postgresql.org with esmtps
 (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256)
	(Exim 4.92)
	(envelope-from <pgsql-hackers-owner+archive@lists.postgresql.org>)
	id 1nYw0X-0002ko-Bv
	for pgsql-hackers@arkaria.postgresql.org; Mon, 28 Mar 2022 20:30:33 +0000
Received: from localhost ([127.0.0.1] helo=malur.postgresql.org)
	by malur.postgresql.org with esmtp (Exim 4.92)
	(envelope-from <pgsql-hackers-owner+archive@lists.postgresql.org>)
	id 1nYw0W-0004D0-9N
	for pgsql-hackers@arkaria.postgresql.org; Mon, 28 Mar 2022 20:30:32 +0000
Received: from makus.postgresql.org ([2001:4800:3e1:1::229])
	by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256)
	(Exim 4.92)
	(envelope-from <sfrost@tamriel.snowman.net>)
	id 1nYw0V-000485-QV
	for pgsql-hackers@lists.postgresql.org; Mon, 28 Mar 2022 20:30:31 +0000
Received: from tamriel.snowman.net ([2001:470:e38f::11])
	by makus.postgresql.org with esmtp (Exim 4.92)
	(envelope-from <sfrost@tamriel.snowman.net>)
	id 1nYw0S-0005Op-Te
	for pgsql-hackers@lists.postgresql.org; Mon, 28 Mar 2022 20:30:30 +0000
Received: by tamriel.snowman.net (Postfix, from userid 1000)
	id 0DD3A5F7A2; Mon, 28 Mar 2022 16:30:28 -0400 (EDT)
Date: Mon, 28 Mar 2022 16:30:27 -0400
From: Stephen Frost <sfrost@snowman.net>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Chapman Flack <chap@anastigmatix.net>,
	Magnus Hagander <magnus@hagander.net>,
	David Steele <david@pgmasters.net>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Postgres restart in the middle of exclusive backup and the
 presence of backup_label file
Message-ID: <20220328203027.GK10577@tamriel.snowman.net>
References: <20220309154209.GU10577@tamriel.snowman.net>
 <CABUevExC0kaA9DY67tEgkbLBf6dmRtU9KgrLwSqVQjYPte7erQ@mail.gmail.com>
 <6228DFE7.4080802@anastigmatix.net>
 <20220309171923.GV10577@tamriel.snowman.net>
 <622900C8.40807@anastigmatix.net>
 <20220309222124.GB180567@nathanxps13>
 <62293417.8070104@anastigmatix.net>
 <20220310000622.GA225595@nathanxps13>
 <622A941A.90909@anastigmatix.net>
 <20220311003834.GA597706@nathanxps13>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="FflW0ym4w2zMdmgG"
Content-Disposition: inline
In-Reply-To: <20220311003834.GA597706@nathanxps13>
User-Agent: Mutt/1.5.24 (2015-08-30)
List-Id: <pgsql-hackers.lists.postgresql.org>
List-Help: <https://lists.postgresql.org/manage/>
List-Subscribe: <https://lists.postgresql.org/manage/>
List-Post: <mailto:pgsql-hackers@lists.postgresql.org>
List-Owner: <mailto:pgsql-hackers-owner@lists.postgresql.org>
List-Archive: <https://www.postgresql.org/list/pgsql-hackers>
Archived-At: 
 <https://www.postgresql.org/message-id/20220328203027.GK10577%40tamriel.snowman.net>
Precedence: bulk


--FflW0ym4w2zMdmgG
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Greetings,

* Nathan Bossart (nathandbossart@gmail.com) wrote:
> On Thu, Mar 10, 2022 at 07:13:14PM -0500, Chapman Flack wrote:
> > Looks like this change to an example in func.sgml is not quite right:
> >=20
> > -postgres=3D# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
> > +postgres=3D# SELECT * FROM pg_walfile_name_offset(pg_backup_stop());
> >=20
> > pg_backup_stop returns a record now, not just lsn. So this works for me:
> >=20
> > +postgres=3D# SELECT * FROM pg_walfile_name_offset((pg_backup_stop()).l=
sn);
>=20
> Ah, good catch.  I made this change in v7.  I considered doing something
> like this
>=20
> 	SELECT w.* FROM pg_backup_stop() b, pg_walfile_name_offset(b.lsn) w;
>=20
> but I think your suggestion is simpler.

I tend to agree.  More below.

> Subject: [PATCH v7 1/1] remove exclusive backup mode
> diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
> index 0d69851bb1..c8b914c1aa 100644
> --- a/doc/src/sgml/backup.sgml
> +++ b/doc/src/sgml/backup.sgml
> @@ -881,19 +873,19 @@ test ! -f /mnt/server/archivedir/00000001000000A900=
000065 &amp;&amp; cp pg_wal/0
>     <listitem>
>      <para>
>       Connect to the server (it does not matter which database) as a user=
 with
> -     rights to run pg_start_backup (superuser, or a user who has been gr=
anted
> +     rights to run pg_backup_start (superuser, or a user who has been gr=
anted
>       EXECUTE on the function) and issue the command:
>  <programlisting>
> -SELECT pg_start_backup('label', false, false);
> +SELECT pg_backup_start(label =3D> 'label', fast =3D> false);
>  </programlisting>
>       where <literal>label</literal> is any string you want to use to uni=
quely
>       identify this backup operation. The connection
> -     calling <function>pg_start_backup</function> must be maintained unt=
il the end of
> +     calling <function>pg_backup_start</function> must be maintained unt=
il the end of
>       the backup, or the backup will be automatically aborted.
>      </para>
> =20
>      <para>
> -     By default, <function>pg_start_backup</function> can take a long ti=
me to finish.
> +     By default, <function>pg_backup_start</function> can take a long ti=
me to finish.
>       This is because it performs a checkpoint, and the I/O
>       required for the checkpoint will be spread out over a significant
>       period of time, by default half your inter-checkpoint interval

Hrmpf.  Not this patch's fault, but the default isn't 'half your
inter-checkpoint interval' anymore (checkpoint_completion_target was
changed to 0.9 ... let's not discuss who did that and missed fixing
this).  Overall though, maybe we should reword this to avoid having to
remember to change it again if we ever change the completion target
again?  Also it seems to imply that pg_backup_start is going to run its
own independent checkpoint or something, which isn't the typical case.
I generally explain what is going on here like this:

By default, <function>pg_backup_start</function> will wait for the next
checkpoint to complete (see ref: checkpoint_timeout ... maybe also
wal-configuration.html).

> @@ -937,7 +925,7 @@ SELECT * FROM pg_stop_backup(false, true);
>       ready to archive.
>      </para>
>      <para>
> -     The <function>pg_stop_backup</function> will return one row with th=
ree
> +     The <function>pg_backup_stop</function> will return one row with th=
ree
>       values. The second of these fields should be written to a file named
>       <filename>backup_label</filename> in the root directory of the back=
up. The
>       third field should be written to a file named

I get that we have <function> and </function>, but those are just
formatting and don't show up in the actual text, and so it ends up
being:

The pg_backup_stop will return

Which doesn't sound quite right to me.  I'd say we either drop 'The' or
add 'function' after.  I realize that this patch is mostly doing a
s/pg_stop_backup/pg_backup_stop/, but, still.

> diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
> index 8a802fb225..73096708cc 100644
> --- a/doc/src/sgml/func.sgml
> +++ b/doc/src/sgml/func.sgml
> @@ -25732,24 +25715,19 @@ LOG:  Grand total: 1651920 bytes in 201 blocks;=
 622360 free (88 chunks); 1029560
>          <parameter>spcmapfile</parameter> <type>text</type> )
>         </para>
>         <para>
> -        Finishes performing an exclusive or non-exclusive on-line backup.
> -        The <parameter>exclusive</parameter> parameter must match the
> -        previous <function>pg_start_backup</function> call.
> -        In an exclusive backup, <function>pg_stop_backup</function> remo=
ves
> -        the backup label file and, if it exists, the tablespace map file
> -        created by <function>pg_start_backup</function>.  In a non-exclu=
sive
> -        backup, the desired contents of these files are returned as part=
 of
> +        Finishes performing an on-line backup.  The desired contents of =
the
> +        backup label file and the tablespace map file are returned as pa=
rt of
>          the result of the function, and should be written to files in the
>          backup area (not in the data directory).
>         </para>

Given that this is a crucial part, which the exclusive mode has wrong,
I'd be a bit more forceful about it, eg:

The contents of the backup label file and the tablespace map file must
be stored as part of the backup and must NOT be stored in the live data
directory (doing so will cause PostgreSQL to fail to restart in the
event of a crash).

> @@ -25771,8 +25749,7 @@ LOG:  Grand total: 1651920 bytes in 201 blocks; 6=
22360 free (88 chunks); 1029560
>          The result of the function is a single record.
>          The <parameter>lsn</parameter> column holds the backup's ending
>          write-ahead log location (which again can be ignored).  The seco=
nd and
> -        third columns are <literal>NULL</literal> when ending an exclusi=
ve
> -        backup; after a non-exclusive backup they hold the desired conte=
nts of
> +        third columns hold the desired contents of
>          the label and tablespace map files.
>         </para>
>         <para>

I dislike saying 'desired' here as if it's something that we're nicely
asking but maybe isn't a big deal, and just saying 'label' isn't good
since we call it 'backup label' elsewhere and we should try hard to be
consistent and clear.  How about:

The second column returns the contents of the backup label file, the
third column returns the contents of the tablespace map file.  These
must be stored as part of the backup and are required as part of the
restore process.

> diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
> index 3c182c97d4..ee3fa148b6 100644
> --- a/src/bin/pg_ctl/pg_ctl.c
> +++ b/src/bin/pg_ctl/pg_ctl.c
> @@ -1069,7 +1069,7 @@ do_stop(void)
>  			get_control_dbstate() !=3D DB_IN_ARCHIVE_RECOVERY)
>  		{
>  			print_msg(_("WARNING: online backup mode is active\n"
> -						"Shutdown will not complete until pg_stop_backup() is called.\n\n"=
));
> +						"Shutdown will not complete until pg_backup_stop() is called.\n\n"=
));
>  		}
> =20
>  		print_msg(_("waiting for server to shut down..."));
> @@ -1145,7 +1145,7 @@ do_restart(void)
>  			get_control_dbstate() !=3D DB_IN_ARCHIVE_RECOVERY)
>  		{
>  			print_msg(_("WARNING: online backup mode is active\n"
> -						"Shutdown will not complete until pg_stop_backup() is called.\n\n"=
));
> +						"Shutdown will not complete until pg_backup_stop() is called.\n\n"=
));
>  		}
> =20
>  		print_msg(_("waiting for server to shut down..."));

This... can't actually happen anymore, right?  Shouldn't we just pull
all of this out?

On a once-over of the rest of the code, I definitely like how much we're
able to simplify things in this area and remove various hacks in things
like pg_basebackup and pg_rewind where we previously had to worry about
backup_label and tablespace_map files being in a live data directory.
I'm planning to spend more time on this and hopefully be able to get it
in for v15.

Thanks!

Stephen

--FflW0ym4w2zMdmgG
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJiQhrjAAoJEO1sijiDR2RV900P/1MAM65hOm3YMLLD9zRVn8pK
jCQX8CFUpJoC8LcInCy1monxDu/yiPNfWcR8F1AYvkvk5AGIh15scXVk+Y/CX2Q5
TEncFUnXDgETnXe/iIY2//V+UG3kva2WBRF26ylO4tmfBgr9EO/+ZgJcTigxU/af
nDBnL3FPAzOWYoEnhaJw0DR8v9zQlGny8X7UJtJ5m25GLIvfuiAjASeHMZijCmr3
uM+vEj9/udr/K0G8iyss64/tvGzb74qGHASg/1xBWJsVEaF85MKpOEOGfdH61nHQ
tqSmdUbi7cAOS12nXc7B16Pz95EUz9RZuqY5vbSJzlXLE+uyXJe/xwcJN+LGvusL
eMqCOWwpoheCvOU2DyHgbJbeUpGCNxbm9ypI6F3E5mUxWpentWcIeJX4LYN3Ai37
y8rycNxNqUsCXkm1hgYc+9LBAPZGFitvi7GvInWfECU+2yqUMXdwm7NGjvY/B8gm
Npx0PkhveKMMAAGPeZLXQNc/SlZbl6aDiX6jYsKiN29WKUO0hUJLS06Uc0xjIKt9
kLn9kuCn5wIKj9PyKDHzPRB9dMuMdAMm//IFnz7TcGqy4q76hv6XGFBOkllfGX3S
aNmqTlwMoTU7mmh1zeigB1/KlC6JzGkOz5clJ9n433JUUbXqHLLkmeXtaT901+Ue
FYc2CgxmRZj2u+HAZtxp
=3eBs
-----END PGP SIGNATURE-----

--FflW0ym4w2zMdmgG--