Date: Wed, 19 Sep 2018 14:20:34 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
Cc: amit.kapila16@gmail.com, robertmhaas@gmail.com, hlinnaka@iki.fi,
	dilipbalaut@gmail.com, pgsql-hackers@postgresql.org
Subject: Re: Problem while setting the fpw with SIGHUP
Message-ID: <20180919052034.GI1650@paquier.xyz>
References: <20180906233728.GR2726@paquier.xyz>
 <20180914072655.GI29332@paquier.xyz>
 <CAA4eK1+Xfx5jD2CHmLPNqXeOzqRLKG9TNr8wfs3-cPf9Ln9RVg@mail.gmail.com>
 <20180918.161542.79085818.horiguchi.kyotaro@lab.ntt.co.jp>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="j/HO4hzKTNbM1mOX"
Content-Disposition: inline
In-Reply-To: <20180918.161542.79085818.horiguchi.kyotaro@lab.ntt.co.jp>
Precedence: bulk


--j/HO4hzKTNbM1mOX
Content-Type: multipart/mixed; boundary="UlxN1C6awaFNesUv"
Content-Disposition: inline


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

On Tue, Sep 18, 2018 at 04:15:42PM +0900, Kyotaro HORIGUCHI wrote:
> My latest patch tries to remove the window by imposing all
> responsibility to apply config file changes to the shared FPW
> flag on the checkpointer. RecoveryInProgress() is changed to be
> called prior to UpdateFullPageWrites on the way doing that.

I still need to look at that in details.  That may be better than what I
am proposing.  At quick glance what I propose is more simple, and does
not need enforcing a checkpoint using SIGINT.

> InRecoery is turned off after the last UpdateFullPageWrite() and
> before SharedRecoveryInProgress is turned off. So it is still
> leaving the window. Thus, checkpointer stops flipping the value
> before SharedRecoveryInProgress's turning off. (I don't
> understand InRecovery condition..) However, this lets
> checkpointer omit reloading after UpdateFullPagesWrite() in
> startup until SharedRecoveryInProgress is tunred off.

That won't matter in this case, as RecoveryInProgress() gets called out
of the critical section in the previous patch I sent, so there is no
failure.  Let's not forget that the first issue is the allocation in the
critical section.  The second issue is that UpdateFullPageWrites may be
called concurrently across multiple processes, which is not something it
is designed for.  The second issue gets addressed in my proposal my
making sure that the checkpointer never tries to update full_page_writes
by himself until recovery has finished, and that the startup process is
the only updater once recovery ends.

> Agreed. "If we need to do that in the start process," we need to
> change the shared flag and issue FPW_CHANGE always when the
> database state is different from configuration file, regardless
> of what StartXLOG() did until the point.

Definitely my mistake here.  Attached is a patch to show what I have in
mind by making sure that the startup process generates a record even
after switching full_page_writes when started normally.  This removes
the condition based on InRecovery, and uses a new argument for
UpdateFullPageWrites() instead.  Your test case,as well as what I do
manually for testing, pass without triggering the assertion.

+   /* DEBUG: cause a reload */
+   {
+       struct stat b;
+       if (stat("/tmp/hoge", &b) == 0)
+       {
+           elog(LOG, "STARTUP SLEEP FOR 1s");
+           sleep(1);
+           elog(LOG, "DONE.");
+           DirectFunctionCall1(pg_reload_conf, 0);
+       }
+   }
The way you patch the backend this way is always nice to see so as it is
easy to reproduce bugs, and it actually helps in reproducing the
assertion failure correctly ;)
--
Michael

--UlxN1C6awaFNesUv
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="fix_updatefullpagewrites_concurrency_v3.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam=
/xlog.c
index 3025d0badb..079e1814c1 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7719,10 +7719,15 @@ StartupXLOG(void)
 	 * Update full_page_writes in shared memory and write an XLOG_FPW_CHANGE
 	 * record before resource manager writes cleanup WAL records or checkpoint
 	 * record is written.
+	 *
+	 * It is safe to check the shared full_page_writes without the lock,
+	 * because there is no concurrently running process able to update it.
+	 * The only other process able to update full_page_writes is the
+	 * checkpointer, still it is unable to do so until recovery finishes.
 	 */
 	Insert->fullPageWrites =3D lastFullPageWrites;
 	LocalSetXLogInsertAllowed();
-	UpdateFullPageWrites();
+	UpdateFullPageWrites(true);
 	LocalXLogInsertAllowed =3D -1;
=20
 	if (InRecovery)
@@ -9693,14 +9698,28 @@ XLogReportParameters(void)
  * Update full_page_writes in shared memory, and write an
  * XLOG_FPW_CHANGE record if necessary.
  *
- * Note: this function assumes there is no other process running
- * concurrently that could update it.
+ * Note: this can be called from the checkpointer, or the startup process
+ * at the end of recovery.  One could think that this routine should be
+ * careful with its lock handling, however this is a no-op for the
+ * checkpointer until the startup process marks the end of recovery,
+ * so only one of them can do the work of this routine at once.
  */
 void
-UpdateFullPageWrites(void)
+UpdateFullPageWrites(bool force)
 {
 	XLogCtlInsert *Insert =3D &XLogCtl->Insert;
=20
+	/*
+	 * Check if recovery is still in progress before entering this critical
+	 * section, as some memory allocation could happen at the end of
+	 * recovery.  There is nothing to do for a system still in recovery.
+	 * Note that the caller may still want to enforce an update to happen.
+	 * One such example is the startup process, which updates full_page_writes
+	 * at the end of recovery.
+	 */
+	if (RecoveryInProgress() && !force)
+		return;
+
 	/*
 	 * Do nothing if full_page_writes has not been changed.
 	 *
@@ -9731,7 +9750,7 @@ UpdateFullPageWrites(void)
 	 * Write an XLOG_FPW_CHANGE record. This allows us to keep track of
 	 * full_page_writes during archive recovery, if required.
 	 */
-	if (XLogStandbyInfoActive() && !RecoveryInProgress())
+	if (XLogStandbyInfoActive())
 	{
 		XLogBeginInsert();
 		XLogRegisterData((char *) (&fullPageWrites), sizeof(bool));
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster=
/checkpointer.c
index 1a033093c5..2084978468 100644
--- a/src/backend/postmaster/checkpointer.c
+++ b/src/backend/postmaster/checkpointer.c
@@ -1347,7 +1347,7 @@ UpdateSharedMemoryConfig(void)
 	 * If full_page_writes has been changed by SIGHUP, we update it in shared
 	 * memory and write an XLOG_FPW_CHANGE record.
 	 */
-	UpdateFullPageWrites();
+	UpdateFullPageWrites(false);
=20
 	elog(DEBUG2, "checkpointer updated shared memory configuration values");
 }
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 421ba6d775..7cfed58399 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -270,7 +270,7 @@ extern void CreateCheckPoint(int flags);
 extern bool CreateRestartPoint(int flags);
 extern void XLogPutNextOid(Oid nextOid);
 extern XLogRecPtr XLogRestorePoint(const char *rpName);
-extern void UpdateFullPageWrites(void);
+extern void UpdateFullPageWrites(bool force);
 extern void GetFullPageWriteInfo(XLogRecPtr *RedoRecPtr_p, bool *doPageWri=
tes_p);
 extern XLogRecPtr GetRedoRecPtr(void);
 extern XLogRecPtr GetInsertRecPtr(void);

--UlxN1C6awaFNesUv--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAluh3KIACgkQnvQgOdby
QH0u3A/8D1fXt/+pmAXXvNQsiKvjGnsZAdr6u+61d+ZYjwlSJKP7pUa/JaMfllWi
LaAYkhSpbTTvADXu6LHa3Znm5yDWHid7VWCwn2RzmELK5rFRU8Ga5YyCzBNPEm17
mojNZZzo25MxKXxIwdbvknpTo+aYFTAGiu79h7u8IPvh3q93OZ1D6cMFz0pQlQkN
x+rIPNuqxqkf08TOWRCvqwGgO6SZlS8ZpBD54Eysy5lyzEsbmNrfhucWWmzo0khq
Qi0TANeUSI487RQguA63nYXh8tfy6V5JaAfQjGwNWSi5ZIECXVpXMrC+KAgNxrqp
xCgLoA4d0h2Ji3elNXD1TsFDsee2XGI8PIu1tnB5sC0kmbPb5Klzt1gsAPFeSzrY
BtKuFBszPTQA5t3j2d2kDPcntlI4Sk4HgGGlW4AgmGuJuEkMsMwtv9HkbdvLHrlu
79QWiVbdPtfQYTm61a/KlZ/BVJoHIjDtbWtfKuJnGQzOV2uISzc+llz9dpgUYJya
Oh4pZeaR8N98sYDR4z2/6ahj9zKv5BsekdHohqf0uXwcl38qMSWeGtx0/xUogcS2
hcfAZg816r3Zp7ZaR/t3Fi7tgzI7V37xnBNrA+ILQlO7FJZHCw2KGngDK4Na0DR9
W/kXotlUAUb4AlRhY83dTyMG2gcU1xiy2/2LN94rQZQ7EFhZYOU=
=/kH0
-----END PGP SIGNATURE-----

--j/HO4hzKTNbM1mOX--