Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 4 Sep 2023 12:54:28 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Thomas Munro <thomas.munro@gmail.com>
Cc: Sergei Kornilov <sk@zsrv.org>, Noah Misch <noah@leadboat.com>,
	Tom Lane <tgl@sss.pgh.pa.us>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>, exclusion@gmail.com,
	pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17928: Standby fails to decode WAL on termination of primary
Message-ID: <ZPVU9FwENbnO6Q++@paquier.xyz>
References: <ZN65RHuB+qNS/G9Q@paquier.xyz>
 <CA+hUKGL0P6CiKuTuxp_bV3peyE61huwPmLEFqVdCt5waqy3QOA@mail.gmail.com>
 <ZN75xA4v14ImLvhy@paquier.xyz>
 <ZOKil9ifJmmAb6di@paquier.xyz>
 <ZPKQA+HHqLVRFBSs@paquier.xyz>
 <1641161693655508@ske52ucy55r5y36k.iva.yp-c.yandex.net>
 <ZPPIf3Uzp9iFrE6M@paquier.xyz>
 <ZPQdHY4WDR+2ejwq@paquier.xyz>
 <222191693740067@6mha55xjikky5vne.iva.yp-c.yandex.net>
 <CA+hUKG+jFN64gKVGcmfNTekqWn3cemRx99-B8DDBaDyzWnfpkw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="OLV1rviX9OPloDkZ"
Content-Disposition: inline
In-Reply-To: 
 <CA+hUKG+jFN64gKVGcmfNTekqWn3cemRx99-B8DDBaDyzWnfpkw@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZPVU9FwENbnO6Q%2B%2B%40paquier.xyz>
Precedence: bulk


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

On Mon, Sep 04, 2023 at 03:20:31PM +1200, Thomas Munro wrote:
> 1.  In the place where we fail to allocate memory for an oversized
> record, I copied the comment about treating that as a "bogus data"
> condition.  I suspect that we will soon be converting that to a FATAL
> error[1], and that'll need to be done in both places.

You mean for the two callers of XLogReadRecordAlloc(), even for the
case where !allow_oversized?  Using a FATAL on non-FRONTEND would be
the quickest fix, indeed, but there are argument for standbys where we
could let these continue, as well.  That would be an improvement over
the always-FATAL on OOM, of course.

> 2.  In this version of the commit message I said we'd only back-patch
> to 15 for now.  After sleeping on this for a week, I realised that the
> reason I keep vacillating on that point is that I am not sure what
> your plan is for the malloc-failure-means-end-of-wal policy ([1],
> ancient code from 0ffe11abd3a).  If we're going to fix that in master
> only but let sleeping dogs lie in the back-branches, then it becomes
> less important to go back further than 15 with THIS patch.

For HEAD, my plan was to give more control to the callers of the WAL
reader, which is not something that could happen in back-branches
because it requires a few structural changes in the reporting the
reader is able to do.  That's the standby vs crash-recovery behavior,
where we can make the startup process not FATAL all the time because
it should just loop forever if it cannot process a record.  It does
not fix the root issue discussed here, of course, but it make the OOM
handling more friendly in some replay cases.

> But if you
> want to be able to distinguish garbage from out-of-memory, and thereby
> end-of-wal from a FATAL please-insert-more-RAM condition, I think
> you'd really need this industrial strength validation in all affected
> branches, and I'd have more work to do, right?  The weak validation we
> are fixing here is the *real* underlying problem going back many
> years, right?

Getting the same validation checks for all the branches would be nice.
FATAL-ing on OOM to force recovery to happen again is a better option
than assuming that it is the end of recovery.  I am OK to provide
patches for all the branches for the sake of this thread, if that
helps.  Switching to a hard FATAL on OOM for the WAL reader in the
backend is backpatchable, but I'd rather consider that on a different
thread once the better checks for the record header are in place.

> I also wondered about strengthening the validation of various things
> like redo begin/end LSNs etc in these tests.  But we can always
> continue to improve all this later...

Agreed.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmT1VPQACgkQnvQgOdby
QH33SA/+PVTO0PBGFpipRfD8R4bke2ro6WXprirJqxNBCgAEnS6X9ffBUsykAVfN
I2YOi6Doy7YvWJ6A/OVcBZXpOH2dreH3iZUV7QgjNH2/jU6/AKd2KduhHu/oggHi
TWouMI9KCYJ8NPUhCoH3Xt+k2rGe/5ng+MDaa974eoo7kel44BRXbG7w3gCbkabP
YAjH7+jNbPMvrhENl/kXq62++/Ir1JHZvlvECBaqZiaTIdU2CyfHMq/JJ11lVAfH
fPyuEngUOS9bafVxr0262ZEi2ndL+weAPtXZ5W1iqyu74EKiNGSRqhvHmDLVM1Or
Gp3bu8zQL3MMROq8sW0FOx0XROM0mFvDQxPYRzSRPAKCnTy9OF/+3ANvrD1O5A3Z
oJ6spvenHR0UBKenP1haRTzatyM6CYX9wCfN5KXcP/eD54/MMShnjDGw80qU5WDm
kkd47VXVu7Ed6qkW5FigOUniMA+Y5Nb8T2EL7zHkzI0pyhXLzj6q104Uhdzs3ZMT
Zajpiy7ichZcsaEd/Vr+/dQcVJq37oEl/0vtg2h/haVPSLFj0eMUexVjM7r8rbe+
Ydle4Y8dxZU4XWXK/b4Jzuzbd6hnGEieLOCFLYwl8F+xfikGvEK9Ix0TFlrmzMzq
p3KB+Jh04luO7GK5bYF0YqPlwD0aRJqRYANCvN4h/cm1CAfguBU=
=Xixe
-----END PGP SIGNATURE-----

--OLV1rviX9OPloDkZ--