Date: Wed, 22 Mar 2017 15:14:51 -0400
From: Stephen Frost <sfrost@snowman.net>
To: Fujii Masao <masao.fujii@gmail.com>
Cc: David Steele <david@pgmasters.net>,
	"Tsunakawa, Takayuki" <tsunakawa.takay@jp.fujitsu.com>,
	Michael Paquier <michael.paquier@gmail.com>,
	Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: PATCH: Make pg_stop_backup() archive wait optional
Message-ID: <20170322191451.GY9812@tamriel.snowman.net>
References: 
 <CA+TgmoZfjF2k6N8Rfga20bXxnRAttuz8N6dvxeOu=t3j0FYYsg@mail.gmail.com>
 <081cbf08-35c1-4580-8c31-3ae70b507fb1@pgmasters.net>
 <0A3221C70F24FB45833433255569204D1F6B3F95@G01JPEXMBYT05>
 <0A3221C70F24FB45833433255569204D1F6B5134@G01JPEXMBYT05>
 <49d70e8e-33bf-2390-e717-ba9562621aa4@pgmasters.net>
 <0A3221C70F24FB45833433255569204D1F6B607B@G01JPEXMBYT05>
 <CAHGQGwHRmE+db96=9FDPLVxWRN0gSf6kU7nM6-WnEKot2OanOg@mail.gmail.com>
 <ee92e800-f983-c3a7-ddae-92acae162832@pgmasters.net>
 <20170322153703.GK9812@tamriel.snowman.net>
 <CAHGQGwFd+7D5UPwLLCW3=5mmDzXOKN56gySRUqdRzaeVK_L2HQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="imYcRqKMHEleKKKf"
Content-Disposition: inline
In-Reply-To: 
 <CAHGQGwFd+7D5UPwLLCW3=5mmDzXOKN56gySRUqdRzaeVK_L2HQ@mail.gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org


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

Fujii,

* Fujii Masao (masao.fujii@gmail.com) wrote:
> On Thu, Mar 23, 2017 at 12:37 AM, Stephen Frost <sfrost@snowman.net> wrot=
e:
> > * David Steele (david@pgmasters.net) wrote:
> >> On 3/21/17 2:34 PM, Fujii Masao wrote:
> >> >The patch basically looks good to me, but one comment is;
> >> >backup.sgml (at least the description for "Making a non-exclusive
> >> >low level backup) seems to need to be updated.
> >>
> >> Agreed.  Added in the attached patch and rebased on 8027556.
>=20
> Thanks for updating the patch!
>=20
> -SELECT * FROM pg_stop_backup(false);
> +SELECT * FROM pg_stop_backup(false [, true ]);
>=20
> I think that it's better to get rid of "[" and "]" from the above because
> IMO this should be the command example that users actually can run.

Using the '[' and ']' are how all of the optional arguments are
specified in the documentation, see things like current_setting() in our
existing documentation:

https://www.postgresql.org/docs/9.6/static/functions-admin.html

> +     If the backup process monitors the WAL archiving process independen=
tly,
> +     the second parameter (which defaults to true) can be set to false to
> +     prevent <function>pg_stop_backup</> from blocking until all WAL is
> +     archived.  Instead, the function will return as soon as the stop ba=
ckup
> +     record is written to the WAL.  This option must be used with cautio=
n:
> +     if WAL archiving is not monitored correctly then the result might b=
e a
> +     useless backup.
>=20
> You added this descriptions into the step #4 in the non-exclusive
> backup procedure.. But since the step #5 already explains how
> pg_stop_backup has to do with WAL archiving, I think that it's better
> to update (or add something like the above descriptions into)
> the step #5. Thought?

That seems pretty reasonable to me.

> +     If the backup process monitors the WAL archiving process independen=
tly,
>=20
> Can we explain "monitor the WAL archiving process" part a bit more
> explicitly? For example, "monitor and ensure that all WAL segment files
> required for the backup are successfully archived".

Sure, makes sense.  I'll add some language along those lines.

> > I've started looking at this.  Seems pretty straight-forward and will
> > try to get it committed later today.
>=20
> Thanks!

My apologies if you had intended to look at committing this, I just
noticed that it hadn't been 'claimed' yet in the CF app and did so to
move forward with it.  I didn't mean to step on anyone's 'toes'.

Thanks!

Stephen

--imYcRqKMHEleKKKf
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature

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

iQIcBAEBCgAGBQJY0s0rAAoJEO1sijiDR2RVcewQALJBJYd3yui2ZqP5NjHB9wC5
KzJ9laQUemamKgvja3JIUca6Zl0o1qLCIdW73/wPcrMzu5HzilByJGkxsDjeKTs6
aHVEfYVyh7NrPw9X+s8Vrt9VmRXN/TFMzEqYpOJWwQkLWvc3nwh5ASBEqM5r51bJ
9fIEVCcLT7Mjj+ie+b4yPAdgDGZbPP/4yZydSUdJPnoQ48pA43mJut5f+d+yC4mW
aTYNrfRSWAJaBvPQv4g+6feCpfv4lk7ofhcbZIvOJ/i7xp6uE0L/5BQlHeWvB+o0
K/txCWIT/pflYlJ9Vf27FxORiwfpXhoDftdDu0FIr/8dCptBAkb3IrTxj+WE1jL3
m18bc0G9dm8lWx04xoPg1fFVieYWmw8fyZ4HjA8Aj4upx2oSW43k5OTwu62eFvfy
1MuS/GMXqoiDwPPEIqQFmUxjEMQaucqsCXnlUVZhXPyMomaM7MHeE6GghieAMMai
+zhzPhzrQFBamnga2NEyBiIRTh6JOptb8mMyx+C7qaIeGufFB1AT1bqEr7SBLVnI
wby34fyaTDu5VMiuBa+k6+JYglLT78PAGhtuN7WGCK/GQwV3Lk5n51BdRyLFYGPs
4TkDcTDm9Cj5PTfLVOqvVCKTABKAwRPh0DVu/eihhb3Sxbzsk601DD8qnsa0MuCr
9mhWGtUYyysxxlYRBG9L
=2TjS
-----END PGP SIGNATURE-----

--imYcRqKMHEleKKKf--