Date: Tue, 3 Nov 2020 18:36:50 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Magnus Hagander <magnus@hagander.net>
Cc: Andres Freund <andres@anarazel.de>, Robert Haas <robertmhaas@gmail.com>,
	Julien Rouhaud <rjuju123@gmail.com>,
	Justin Pryzby <pryzby@telsasoft.com>,
	Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Masahiko Sawada <sawada.mshk@gmail.com>
Subject: Re: Online checksums verification in the backend
Message-ID: <20201103093650.GA2298@paquier.xyz>
References: <20201023072832.GE5180@paquier.xyz>
 <CAOBaU_bznSonMWqF+N3A8ZOcXx=G4ovwvfgmyES9rmGh5+rV5Q@mail.gmail.com>
 <20201023090630.GA26347@paquier.xyz>
 <20201027070746.GD28445@paquier.xyz>
 <CAOBaU_bM2tCRAOpQyTSaMLT--=YLKGsyZ-tzWTn25kGbaqrevA@mail.gmail.com>
 <20201028050852.GF28445@paquier.xyz>
 <20201029181729.2nrub47u7yqncsv7@alap3.anarazel.de>
 <CA+TgmoZdA08hSWeVBCT_EywBRqH=2Ft7r7QB0+igi1pgidEnBA@mail.gmail.com>
 <20201102193457.fc2hoen7ahth4bbc@alap3.anarazel.de>
 <CABUevEy+vPu3_FPf+nKizmEjQm-hrKt_h3GgqS_Ohknq7yNFuQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="LZvS9be/3tNcYl/X"
Content-Disposition: inline
In-Reply-To: 
 <CABUevEy+vPu3_FPf+nKizmEjQm-hrKt_h3GgqS_Ohknq7yNFuQ@mail.gmail.com>
Precedence: bulk


--LZvS9be/3tNcYl/X
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Nov 03, 2020 at 09:31:20AM +0100, Magnus Hagander wrote:
> On Mon, Nov 2, 2020 at 8:35 PM Andres Freund <andres@anarazel.de> wrote:
>> On 2020-11-02 12:35:30 -0500, Robert Haas wrote:
>>> It feels really confusing to me that the user-exposed function here is
>>> called pg_relation_check_pages(). How is the user supposed to
>>> understand the difference between what this function does and what the
>>> new verify_heapam() in amcheck does? The answer is that the latter
>>> does far more extensive checks, but this isn't obvious from the SGML
>>> documentation, which says only that the blocks are "verified," as if
>>> an end-user can reasonably be expected to know what that means. It
>>> seems likely to lead users to the belief that if this function passes,
>>> they are in good shape, which is extremely far from being true. Just
>>> look at what PageIsVerified() checks compared to what verify_heapam()
>>> checks.

The cases of verify_heapam() are much wider as they target only one
AM, while this stuff should remain more general.  There seems to be
some overlap in terms of the basic checks done by bufmgr.c, and
the fact that you may not want to be that much intrusive with the
existing buffer pool as well when running the AM checks.  It also
seems to me that the use cases are quite different for both, the
original goal of this thread is to detect physical corruptions for all
AMs, while verify_heapam() looks after logical corruptions in the way
heap is handled.

>> Yea I had similar thoughts, it should just be called
>> pg_checksum_verify_relation() or something.
>=20
> +1.

I mentioned that upthread, is there really a dependency with checksums
here?  There are two cases where we can still apply some checks on a
page, without any need of checksums:
- The state of the page header.
- Zeroed page if pd_upper is 0.  Those pages are valid, and don't have
a checksum computed.
So it seems to me that when it comes to relation pages, then the
check of a page should answer to the question: is this page loadable
in shared buffers, or not?

>>> In fact, I would argue that this functionality ought to live in
>>> amcheck rather than core, though there could usefully be enabling
>>> functions in core.
>>
>> I'm not really convinced by this though. It's not really AM
>> specific - works for all types of relations with storage; don't really
>> object either...
>=20
> Yeah, I'm not sure about that one either. Also what would happen
> if/when we get checksums on things that aren't even relations? (though
> maybe that goes for other parts of amcheck at some point as well?)

I also thought about amcheck when looking at this thread, but it did
not seem the right place as this applies to any AM able that could
load stuff into the shared buffers.
--
Michael

--LZvS9be/3tNcYl/X
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl+hJLIACgkQnvQgOdby
QH0a6hAAiqMELsOM3tKqzMycxAl3ktXFyUBqNfr4KzHKfpBd1SQ95xhiSeVniGAA
xV3QTswQ2C0GJlZ/QlkX2B1Y60vN338ajz2sjCKnCtZL0oxHMgEwtavqM9+f9KiI
B2W9oWHgyd+7x6z7krxp9KHC7MK0zeNS42dSCnDH6t8oCOH3ibi0weQ78ByC0EGF
oEto028sXxf862YvhAYq8GIY1ngQhAJmJqTR2qNLDbU4Ing7mwTBzdCjLMcVZjaa
jgI2WG4vBAvxp7EXOFySpgzMKSwKJWNI0xb6wiVxmXdJpmiTTj3PHrjh7Ppdmh9K
9OKwkLvfDGJFLGXgAdarSwwh1GquJb1UTjL5bglKiuOaeAUXgjBTvN+Eiqjc5cbE
adLS79Hodptl11LfSyd8TtwLDMQkJMfSVqa7bX0x/z9SVGi+m9li+FCKqlzbZpH0
K1/QEZ/9EoQsXFZd2DpTLKW/8M2JPRCC4KLX3/9lMnDYDuvEHwqk4Z6WogtvOJUM
xAWmwJ3CEXewmf2Jq4lwsNO6d9Ig1LibaUEYcs/GeuSKMzaaHvMqOyyKkSQ1LVGp
F4nF1QgtSpTlpuQN4+yBWbGPGj7JNNakOE6nO4Tc19aZiNh4az9G35GXGbaWTJni
oCSOHDu3xZsnJ6wwu27YPOywWWFebEGybKBF563bHsN0eO8b0LQ=
=DT5Q
-----END PGP SIGNATURE-----

--LZvS9be/3tNcYl/X--