Date: Mon, 19 Oct 2020 16:52:48 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Julien Rouhaud <rjuju123@gmail.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>,
	Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
	Robert Haas <robertmhaas@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Masahiko Sawada <sawada.mshk@gmail.com>
Subject: Re: Online checksums verification in the backend
Message-ID: <20201019075248.GD9612@paquier.xyz>
References: 
 <CAOBaU_a-=xi6mPF5imNKMFRAoBEpkYkkAoW9Ss+XA4qApZ6WqQ@mail.gmail.com>
 <20200916034558.GB11110@paquier.xyz>
 <CAOBaU_Y_A0m5x2QwQ=z6aNf=M-uT1aFFVLeMcHfMDS8z+KrTTw@mail.gmail.com>
 <20201001050717.GK8130@paquier.xyz>
 <CAOBaU_ZV9hEBo2z=0htszBw2PFSkp=kLmjUTV5PzxQjCKB9zHA@mail.gmail.com>
 <CAOBaU_Yr1DHTC3w1W_KCOr-vsAOgtXFGWM7qNhQAyQnjW_sTrQ@mail.gmail.com>
 <CAOBaU_Yv9crHtC45AaEwQscvoQEgckEa=F_ttJ4T3x3D6QSB8A@mail.gmail.com>
 <CAOBaU_bsMjyqpwd5H=U9VkiQQAGaiak2ouD-DRO7849Drj5crw@mail.gmail.com>
 <20201019023918.GB9612@paquier.xyz>
 <CAOBaU_YZTowoJJ+NzsBieV0K5OvpOS+dAMD1cmz25hxtVZpQqA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="GyRA7555PLgSTuth"
Content-Disposition: inline
In-Reply-To: 
 <CAOBaU_YZTowoJJ+NzsBieV0K5OvpOS+dAMD1cmz25hxtVZpQqA@mail.gmail.com>
Precedence: bulk


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

On Mon, Oct 19, 2020 at 11:16:38AM +0800, Julien Rouhaud wrote:
> On Mon, Oct 19, 2020 at 10:39 AM Michael Paquier <michael@paquier.xyz> wr=
ote:
>> Somewhat related to the first point, NoComputedChecksum exists
>> because, as the current patch is shaped, we need to report an existing
>> checksum to the user even for the zero-only case.
>=20
> I'm not sure that I understand your point.  The current patch only
> returns something to users when there's a corruption.  If by
> "zero-only case" you mean "page corrupted in a way that PageIsNew()
> returns true while not being all zero", then it's a corrupted page and
> then obviously yes it needs to be returned to users.

Sorry for the confusion, this previous paragraph was confusing.  I
meant that the reason why NoComputedChecksum exists is that we give up
on attempting to calculate the checksum if we detect that the page is
new, but failed the zero-only test, and that we want the users to know
about this special case by setting this expected checksum to NULL for
the SRF.

>>  So, wouldn't it be better to just rely on PageIsVerified()
>> (well it's rather-soon-to-be extended flavor) for the checksum check,
>> the header sanity check and the zero-only check?  My point is to keep
>> a single entry point for all the page sanity checks, so as base
>> backups, your patch, and the buffer manager apply the same things.
>> Base backups got that partially wrong because the base backup code
>> wants to keep control of the number of failures and the error
>> reports.
>=20
> I'm fine with it.

Thanks.

>>  Your patch actually wishes to report a failure, but you want
>> to add more context with the fork name and such.  Another option we
>> could use here is to add an error context so as PageIsVerified()
>> reports the WARNING, but the SQL function provides more context with
>> the block number and the relation involved in the check.
>=20
> Also, returning actual data rather than a bunch of warnings is way
> easier to process for client code.  And as mentioned previously having
> an API that returns a list of corrupted blocks could be useful for a
> single-page recovery feature.

No issues with reporting the block number and the fork type in the SRF
at least of course as this is helpful to detect the position of the
broken blocks.  For the checksum found in the header and the one
calculated (named expected in the patch), I am less sure how to put a
clear definition on top of that but we could always consider that
later and extend the SRF as needed.  Once the user knows that both do
not match, he/she knows that there is a problem.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl+NRdAACgkQnvQgOdby
QH3Z2Q/+MnnErwQgd6rGaLdsUWztFLyyYqCCZHFoKlmYBou7M34EDS/kH8CjiSkg
pwCD9enJkZljQF64CDw0j+036l2yC19iRWts5kTpEW76Oa904cB8HhSjW1U9vfK+
tZuEgamsC+1h6ALh62hKR2LnBZ+Hkh3MMZOkSatemUNXbz0o6iRtfVaIGLYDJFo2
UD/wWaRdM8BPRC5qkhulGSds0/qRYjT+YTGOSsZXH0IhNQfJj7+CPpBmb+7uDZdm
ew02No1DhPXLUbAd1ZhaL5ILe2mU85+lpu17Z+fEOLdSs2Vor6lhurKfgmhlNYCP
hqFKUWTt+QcbkypCI9KrwkyJLqni/i9FeU97lr4jDl4KqLR9YB54Px2hJn0iqdL9
MMYbpEvAMKP1ZugzwyUQOSRO2r13l6KyyyT91I5e6dM0781O+Mz75K/ROEj/ohZa
BMde67vWplOr1TfqIne0q1kUZlUcpi5r5cm6RbBuVL3a2ay2E8ZBK+Fe19zEq3E/
xobft39qMb1CVDcS6I9UNTRVqAYgCAPyM67FvNP34Y5KGWMBg7zcZrL5Im6WpmFS
x7Q5xz+ZNWkd1aZZxH7wW3adQHpNBOmeVJtyhSIYlQ7/qm15iruXviJbc1H4i0Nc
9ECiEMojHEZmM7msLpQQ05tiIbUYvGgol6O755d/fgkBh1SVQLI=
=La61
-----END PGP SIGNATURE-----

--GyRA7555PLgSTuth--