Date: Thu, 30 Aug 2018 06:39:11 -0700
From: Michael Paquier <michael@paquier.xyz>
To: Alexander Kukushkin <cyberdemn@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	Dmitry Dolgov <9erthalion6@gmail.com>,
	pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15346: Replica fails to start after the crash
Message-ID: <20180830133911.GA15446@paquier.xyz>
References: <20180823041420.GA1158@paquier.xyz>
 <CAFh8B==4gpwor6ODercru0iifqwd0nQrdRvXzmorWkMRQa4q=g@mail.gmail.com>
 <CAFh8B=nHdsX83hTZMqGqbht39ud+DRQOv4_Te+xu3DuPssQbZA@mail.gmail.com>
 <CAFh8B=mYGP1J8ueHKU40Dr4bNBUPJj1rTbZ6Wpg4gR30uCRunA@mail.gmail.com>
 <20180829061723.GA5903@paquier.xyz>
 <CAFh8B=kuFyBsth-1wuPqncnTiCa7reKzCz=RzO1X-Y78U74F5g@mail.gmail.com>
 <20180829121051.GC5903@paquier.xyz>
 <CAFh8B==_6cY1f7rF4oxK+wjpWKaYzs96Q6Tn5=4QRWJiVnjmDg@mail.gmail.com>
 <20180829205436.GE5903@paquier.xyz>
 <CAFh8B=n-NSo2Ktz_DG3W4FAFr2xHYr7FyRNdOF6g7T2o1-CD4w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="XOIedfhf+7KOe/yw"
Content-Disposition: inline
In-Reply-To: 
 <CAFh8B=n-NSo2Ktz_DG3W4FAFr2xHYr7FyRNdOF6g7T2o1-CD4w@mail.gmail.com>
Precedence: bulk


--XOIedfhf+7KOe/yw
Content-Type: multipart/mixed; boundary="huq684BweRXVnRxX"
Content-Disposition: inline


--huq684BweRXVnRxX
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Aug 30, 2018 at 10:55:23AM +0200, Alexander Kukushkin wrote:
> Bgwriter itself never changes updateMinRecoveryPoint to true and boom,
> we can get a lot of pages written to disk, but minRecoveryPoint in the
> pg_control won't be updated!

That's indeed obvious by reading the code.  The bgwriter would be
started only once a consistent point has been reached, so the startup
process would have normally already updated the control file to the
consistent point.  Something like the attached should take care of the
problem.  As the updates of the local copy of minRecoveryPoint strongly
rely on if the startup process is used, I think that we should use
InRecovery for the sanity checks.

I'd like to also add a TAP test for that, which should be easy enough if
we do sanity checks by looking up at the output of the control file.
I'll try to put more thoughts on that.

Does it take care of the problem?
--
Michael

--huq684BweRXVnRxX
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="min-consistent-update-v1.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam=
/xlog.c
index 493f1db7b9..f9c52fb9d8 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -2723,9 +2723,14 @@ UpdateMinRecoveryPoint(XLogRecPtr lsn, bool force)
 	 * i.e., we're doing crash recovery.  We never modify the control file's
 	 * value in that case, so we can short-circuit future checks here too. The
 	 * local values of minRecoveryPoint and minRecoveryPointTLI should not be
-	 * updated until crash recovery finishes.
+	 * updated until crash recovery finishes.  We only do this for the startup
+	 * process replaying WAL which needs to keep clean references of
+	 * minRecoveryPoint until it is made sure that it has replayed all WAL
+	 * available for crash recovery.  Other processes would be started after
+	 * the consistency point has been reached on a standby, so it would be
+	 * actually safe to update the control file in this case.
 	 */
-	if (XLogRecPtrIsInvalid(minRecoveryPoint))
+	if (XLogRecPtrIsInvalid(minRecoveryPoint) && InRecovery)
 	{
 		updateMinRecoveryPoint =3D false;
 		return;
@@ -2737,7 +2742,9 @@ UpdateMinRecoveryPoint(XLogRecPtr lsn, bool force)
 	minRecoveryPoint =3D ControlFile->minRecoveryPoint;
 	minRecoveryPointTLI =3D ControlFile->minRecoveryPointTLI;
=20
-	if (force || minRecoveryPoint < lsn)
+	if (XLogRecPtrIsInvalid(minRecoveryPoint))
+		updateMinRecoveryPoint =3D false;
+	else if (force || minRecoveryPoint < lsn)
 	{
 		XLogRecPtr	newMinRecoveryPoint;
 		TimeLineID	newMinRecoveryPointTLI;

--huq684BweRXVnRxX--

--XOIedfhf+7KOe/yw
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAluH838ACgkQnvQgOdby
QH2NMRAAqLIIGiuwyLSZTYrRsYz3VVIHx7evDtgvDsRbPDyh66wFtuBg8wjvx04u
dxW1IvgWh+yahJbIP1p/kCEJf0zHCcn11WQb1Nym5eoJQFZOmi7yZt8kAmLCFHJR
nb0LqSa+H1e6/Suytl5ySD2gFnsEZGU1jkMJ4qzKSpeA1qB2cIu9qKgMXFj83M6l
XToH06JdBYG9e4KDpsrZ3TlfGJpNyHhD8UrzfncNI9rLEMTyQBg9wkHRUwBaT8W5
h61b3o6CPHw3Zz6d0z2NTcmNctjZJGgvml26YyUllcB8SWBGHJMz4PQRZdbfxFpB
XvLYYEAZMnO0FaUo27+jRxuWh4ZzNSdpVR8j6O70WKJB8mwKrRGZOs4bzdjLl0KR
XQeAAwTxozzA1z53cl0JF6WEzhZoBeIRBrmAsTxvP21zS7zUyxwgysJwycieW0iH
gtdLZvc/AGjKeJ+ocaUumADcSY8M5HAg5w6DjcBiE19TcKOJ2lS/TYc/Tbn/NUcF
m1va14XGHJiKXfNF2gYeRfAVviS85TxBl18CqGFeCfjI4kGue75nzZgNGnq6skE+
C7C0if1WO7XhVt7M6zzk1QYzq89VEvjWc8dgs2sNqhoiVRClk+3NGMaoRN0BZ+ee
rOzGzueaeKSGfr03TrHztp07KsJRcknDlhzExI6PFM+9pbH/6nw=
=g7Ou
-----END PGP SIGNATURE-----

--XOIedfhf+7KOe/yw--