Date: Fri, 21 Sep 2018 13:04:07 +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: <20180921040407.GF1338@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>
 <20180919052034.GI1650@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="oOB74oR0WcNeq9Zb"
Content-Disposition: inline
In-Reply-To: <20180919052034.GI1650@paquier.xyz>
Precedence: bulk


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

On Wed, Sep 19, 2018 at 02:20:34PM +0900, Michael Paquier wrote:
> 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.
>=20
> 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.

I have finally looked at the patch set from Horiguchi-san here:
https://www.postgresql.org/message-id/20180828.193436.253621888.horiguchi.k=
yotaro@lab.ntt.co.jp

And actually this is very close to what my proposal does, except for a
couple of caveats.  Here are my notes:
1) The issue with palloc() happening in critical sections is able to go
away, by making the logic behind UpdateFullPageWrites() more complicated
than necessary.  With the proposed patch, UpdateFullPageWrites() never
gets called by the checkpointer until the startup process has done its
business.  I would have believed that keeping the check to
RecoveryInProgress() directly in UpdateFullPageWrites() makes the logic
more simple.  That's actually what my proposal does.  With your patch,
it would be even possible to add an assertion so as this never gets
called while in recovery.
2) At the end of recovery, if there is a crash before the checkpointer
is able to update the shared parameters it needs to work on
away, then no XLOG_CHANGE_PARAMETER record is generated.  This is not a
problem for normal cases, but there is one scenario where this is a
problem: at the end of recovery if the bgwriter is not started, then the
startup process creates a checkpoint by itself, and it would miss the
record generation.
3) SIGINT is abused of, in such a way that the checkpointer may generate
two checkpoints where only one is needed post-recovery.
4) SyncRepUpdateSyncStandbysDefined() would get called even without
SIGHUP being reached.  This feels also like a future trap waiting to
bite as well.

When I see your patch, I actually see the same kind of logic as what I
propose which is summarized in two points, and that's a good thing:
a) Avoid the allocation in the critical section.
b) Avoid two processes to call UpdateFullPageWrites at the same time.

Now the points mentioned above make what you are proposing weaker in my
opinion, and 2) is an actual bug, side effect of the proposed patch.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlukbbcACgkQnvQgOdby
QH2HlQ/+OuGTZ/9E0Hmiwei5mhjAe8EpQhTYHAwy6pNMSH3AV1SNMrkWIvV60402
f10EHXeJc+5SHDfgPgssgnzm8Y2goDRBoWcJ4FC8AwboNX8K7zlFm7ZMx2y58Rbc
/bnrbMcfU8DTolKyEIip2ikc23RxNMrC9qnM5Do5Yal4/baPjj5Ji1qawAsKn6cX
++pxOWtChczzKGGiGPVGJMX/3P6MQaQBCr1O/B2ihCiOfFBZvquoemmdWdAtITOk
O102HDfgWya6ldGjmKxcV+oxT5fZEpn9BhIhBnqFhjGHS5CBHtmBsTO2xmN5MEW4
9lFVYRSq1AxfJEfTCWnprbpWcVAlJdSakgtF1q1YcihSDn4cFx3PdvbwS3Da2FAE
nUgqxXQYZWPhCHBMwpEvDSof9OCGgDyUffZQ08st3Rb/I6KNV7IG0GBMvXSU5q1Z
UNO/HaA+VJ3wu2U2U/JxMU9wDvy2AzmXHla5nGbtoAQw9VfBWyRsSKmF4vO+mPhJ
hMxCOVoh2tiWVci8rux2dp2uQy2BwOQ9SP3nG7Fu+8ODyGQtNPDDFZPqipXQ7nos
SLxywlOzhH+XRRp0JT2Ix1EbO4JBgz1TfTauoc6IckfnlahF4oDzVF0e8aZkIA66
D8LqEtone8+Xsv9QU2kvHYL3t5pJkfaHdpof2bWsooIPSBUzHBc=
=chey
-----END PGP SIGNATURE-----

--oOB74oR0WcNeq9Zb--