Date: Tue, 27 Mar 2018 16:46:30 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Problem while setting the fpw with SIGHUP
Message-ID: <20180327074630.GD9940@paquier.xyz>
References: 
 <CAFiTN-u4BA8KXcQUWDPNgaKAjDXC=C2whnzBM8TAcv=stckYUw@mail.gmail.com>
 <20180313080404.GE23071@paquier.xyz>
 <20180316052354.GI2666@paquier.xyz>
 <CAFiTN-vT5m4=CLuZ6pGtNE=_+2rN5nzE6Cd9g9GWv1rLsnyPcQ@mail.gmail.com>
 <20180320055608.GB13368@paquier.xyz>
 <CAFiTN-vm=SYavZUweBvR4tkjSdzG=KTuLQuyQH3MWBfa=aXY+w@mail.gmail.com>
 <20180323074956.GB7070@paquier.xyz>
 <CAFiTN-ucRYDSpYT=ZzDJ9CwiG+YmZ-Hapg8XCLU1XJqcu+oKVA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="sDKAb4OeUBrWWL6P"
Content-Disposition: inline
In-Reply-To: 
 <CAFiTN-ucRYDSpYT=ZzDJ9CwiG+YmZ-Hapg8XCLU1XJqcu+oKVA@mail.gmail.com>
Precedence: bulk


--sDKAb4OeUBrWWL6P
Content-Type: multipart/mixed; boundary="ILuaRSyQpoVaJ1HG"
Content-Disposition: inline


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

On Mon, Mar 26, 2018 at 02:32:22PM +0530, Dilip Kumar wrote:
> On Fri, Mar 23, 2018 at 1:19 PM, Michael Paquier <michael@paquier.xyz>
> wrote:
> In StartupXLOG, just before the CreateCheckPoint() call,  we are calling
> LocalSetXLogInsertAllowed().  So, I am thinking can we just remove
> InitXLogInsert from CreateCheckpoint. And, we don't need to move it to
> bootstrap.c.  Or am I missing something?

I have finally been able to spend more time on this issue, and checked
for a couple of hours all the calls to RecoveryInProgress() that could
be triggered within a critical section to see if the move I suggested
previously is worth it ot not as this would cost some memory for
standbys all the time, which would suck for many read-only sessions.

There are a couple of calls potentially happening in critical sections,
however except for the one in UpdateFullPageWrites() those are used for
sanity  checks in code paths that should never trigger it, take
XLogInsertBegin() for example.  So with assertions this would trigger
a failure before the real elog(ERROR) message shows up.

Hence, I am changing opinion still I think that instead of the patch you
proposed first we could just do a call to InitXLogInsert() before
entering the critical section.  This is also more consistent with what
CreateCheckpoint() does.  That's also less risky for a backpatch as your
patch increases the window between the beginning of the critical section
and the real moment where the check for RecoveryInProgress is needed.  A
comment explaining why the initialization needs to happen is also
essential.

All in all, this would give the simple patch attached.

Thoughts?
--
Michael

--ILuaRSyQpoVaJ1HG
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="0001-Fix-WAL-insert-when-updating-full_page_writes-for-ch.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 0fc3878375890aef5c0a6dc57d73c30aa97c88df Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Tue, 27 Mar 2018 16:38:37 +0900
Subject: [PATCH] Fix WAL insert when updating full_page_writes for
 checkpointer

At startup or when receiving a SIGHUP signal to update its configuration,
the checkpointer may need to update the shared memory for
full_page_writes and needs to write a WAL record about it.  However,
just after a promotion, there is a small window where it is possible
that the memory context in charge of building WAL records has not been
correctly initialized, leading to a crash of the checkpointer.  With
assertions enabled, this causes a failure when allocating memory in a
critical section.  And this fails when trying to build the FPW record
without assertions.

Report and patch by Dilip Kumar, reviewed by Michael Paquier.

Discussion:
    https://postgr.es/m/CAFiTN-u4BA8KXcQUWDPNgaKAjDXC%3DC2whnzBM8TAcv%3Dstc=
kYUw%40mail.gmail.com
---
 src/backend/access/transam/xlog.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam=
/xlog.c
index cb9c2a29cb..aae1131977 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9543,6 +9543,16 @@ UpdateFullPageWrites(void)
 	if (fullPageWrites =3D=3D Insert->fullPageWrites)
 		return;
=20
+	/*
+	 * Initialize InitXLogInsert working areas before entering the critical
+	 * section.  Normally, this is done by the first call to
+	 * RecoveryInProgress() or LocalSetXLogInsertAllowed(), but it can
+	 * be possible that this has not been initialized yet for a checkpointer
+	 * which updates the value of full_page_writes for a received SIGHUP or
+	 * at process startup.
+	 */
+	InitXLogInsert();
+
 	START_CRIT_SECTION();
=20
 	/*
--=20
2.16.3


--ILuaRSyQpoVaJ1HG--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlq59tYACgkQnvQgOdby
QH3QDQ/+LDPjDOakS6NnfiiSoMwMR3PeFycAMiyRUXEP/dR9TrdJR1DRFNNtrOQW
qPIEEU65IgKpQ/ktSCdEVLh35mvOzHLNBtnI3+03zj9KwBTx6a/Xtrz09fX7RbDM
WyRZUzPrb//SB0Tibgcmj2t9GZMs96RGHWR5mPjsdVNpTHOgX5EQlHFu+9EInWLd
LUj02jsvWj3CkF9w8ahhMXpaK7aPgFVNCoi6LYnQSQhEp6R8Qs945L36zOT9C8Xd
TvGRWrLl9rGq1nGy3xx20eYBgmc8hVlr9p0jxWvZL7Yc1klzoJW9NolKba7PKiMG
b2v4GDfwftXvY7dteXuD5MSaRdYF2Du5rjUipThv7IwVIDsNgSLZ4fq7gK62j0fo
O9ptwyJbAMzt71aGJ3sXTJqomH55d2WuvgqdXW4wHvhm+RmSPGrOR5tY9siOnm2Z
292dLNMfxY3XDkXdF8RGhNkj105sQpByFbNCaUtYsIt9HM6/jDvcQD2NEE9rrjYX
9+Dubpz+OaHltG0qSAxnCDbdTbcE0hIdcr6OT6qQHyFBm3+0KSuWXq5CjQcqyH2U
xMBzQKnnzSHYy6FAwHup8L2eqOdI6XBTgKMJrNFQw8cvEwFK5mWVCPxq1SGa9uWs
ojPFWR1WPAH+AmOF2q5fMrWcPgnqax97LC8nBG2AAhTKpdkqdSY=
=u/1z
-----END PGP SIGNATURE-----

--sDKAb4OeUBrWWL6P--