Date: Tue, 9 Nov 2021 09:53:15 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: bossartn@amazon.com, david@pgmasters.net,
	peter.eisentraut@2ndquadrant.com, andres@anarazel.de,
	pgsql-hackers@lists.postgresql.org, jtc331@gmail.com,
	robertmhaas@gmail.com
Subject: Re: Make mesage at end-of-recovery less scary.
Message-ID: <YYnGe0tROh+1SqIc@paquier.xyz>
References: <b432bad3-e379-8a3a-6a35-1d47c116f2de@pgmasters.net>
 <20210304.155039.2020434514871976864.horikyota.ntt@gmail.com>
 <DD96F20D-1118-4C8E-8B04-D1F50320B0C7@amazon.com>
 <20211108.145946.1513355777186578917.horikyota.ntt@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="r7EJ/PzYTNmAczy4"
Content-Disposition: inline
In-Reply-To: <20211108.145946.1513355777186578917.horikyota.ntt@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/YYnGe0tROh%2B1SqIc%40paquier.xyz>
Precedence: bulk


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

On Mon, Nov 08, 2021 at 02:59:46PM +0900, Kyotaro Horiguchi wrote:
> While checking the behavior for the case of missing-contrecord, I
> noticed that emode_for_corrupt_record() doesn't work as expected since
> readSource is reset to XLOG_FROM_ANY after a read failure. We could
> remember the last failed source but pg_wal should have been visited if
> page read error happened so I changed the function so that it treats
> XLOG_FROM_ANY the same way with XLOG_FROM_PG_WAL.

FWIW, I am not much a fan of assuming that it is fine to use
XLOG_FROM_ANY as a condition here.  The comments on top of
emode_for_corrupt_record() make it rather clear what the expectations
are, and this is the default readSource.

> (Otherwise we see "LOG: reached end-of-WAL at .." message after
>  "WARNING: missing contrecord at.." message.)

+      /* broken record found */
+      ereport(WARNING,
+                      (errmsg("redo is skipped"),
+                       errhint("This suggests WAL file corruption. You might need to check the database.")));
This looks rather scary to me, FWIW, and this could easily be reached
if one forgets about EndOfWAL while hacking on xlogreader.c.
Unlikely so, still.

+       report_invalid_record(state,
+                             "missing contrecord at %X/%X",
+                             LSN_FORMAT_ARGS(RecPtr));
Isn't there a risk here to break applications checking after error
messages stored in the WAL reader after seeing a contrecord?

+   if (record->xl_tot_len == 0)
+   {
+       /* This is strictly not an invalid state, so phrase it as so. */
+       report_invalid_record(state,
+                             "record length is 0 at %X/%X",
+                             LSN_FORMAT_ARGS(RecPtr));
+       state->EndOfWAL = true;
+       return false;
+   }
This assumes that a value of 0 for xl_tot_len is a synonym of the end
of WAL, but cannot we have also a corrupted record in this case in the
shape of xl_tot_len being 0?  We validate the full record after
reading the header, so it seems to me that we should not assume that
things are just ending as proposed in this patch.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmGJxnsACgkQnvQgOdby
QH0DZA/+JDdfv80/pqYRefNqedAGK2E+gUPCLfQLlitg5Kwvj4oRP1ETcCV+WBJT
Qtr/tc2aL1eOwphdWHGeB2DEdKz4CSLFDNr2oy0zo0ts0p0q01HSv2lGfaEUy6qT
uLuTKpzvqMvlf6TudVqh94LaQ0AnL2I4e6a6TQsv0Mpbr5E2TDenO6NQ0wSZ6HLD
dKeRtgG1CI4XgpXdMrky7vtuie8WnMqcT5bXWZ32nBymhiS3eqvPuawoCYamuCsN
CeHVOQjHt+wDpBCeEl7hipi8UAfVLFZBVJc5B3koz5YsDv+9EWEwwa7YTuJmJpL0
ENjKR8S6zQO+cijdW3kevH8AYHhFrp+1bkLCYRiJ6XUYHA96au/V/WMlCyGXDv6u
CKXNc5XWkupeB7ekx0vLAFqNelioqj8oCrh4dttzqXGS5uXbxWR/RojH3iHv4EOM
f/uzx5N7QKOUxmZNh0sLgAqov8NRBQNAJMLb7RMdGaB+zT49J5uE+OOw/II8/JlZ
gj+w1IHjTvbtzwGUv6nEtRrU7CCJM+DYw3Mi36NPr7fPFw/XXY4Z1vdrPFT/w8I/
UTz/eYMqPJ4yAXVylUNOPiwh5mrjeX+4UFtKjUhaeOKqgUjIJ8XwiYVX4pV3gy89
InG92BQAWnk8qEcz3QHNNzINi9kjHrIVWSIySJEhQMwwOJfyqeI=
=MQpK
-----END PGP SIGNATURE-----

--r7EJ/PzYTNmAczy4--