Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\))
Subject: Re: new heapcheck contrib module
From: Mark Dilger <mark.dilger@enterprisedb.com>
In-Reply-To: 
 <CA+TgmoZT3kCOaKq6=m5wHGKh_9XQDbCC1LZXL6w_+NMB-n1+mw@mail.gmail.com>
Date: Sun, 2 Aug 2020 20:17:25 -0700
Cc: Amul Sul <sulamul@gmail.com>,
 Dilip Kumar <dilipbalaut@gmail.com>,
 Peter Geoghegan <pg@bowt.ie>,
 Andres Freund <andres@anarazel.de>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <53B5B281-81BF-40FD-9903-EBAA5193506B@enterprisedb.com>
References: <12ED3DA8-25F0-4B68-937D-D907CFBF08E7@enterprisedb.com>
 <20200420194210.sevnpi4lyghoanxi@alap3.anarazel.de>
 <23E60C3C-C20A-4508-84AC-E0CEA59E784A@enterprisedb.com>
 <CA+TgmobAm8vB1MNQ1taO1et+HxTN8ULnQvEG+grNDqNQTBjbtA@mail.gmail.com>
 <F845AC50-A479-43F3-ADFB-861A4D2AD287@enterprisedb.com>
 <5F743835-3399-419C-8324-2D424237E999@enterprisedb.com>
 <CAFiTN-syyHc3jZoou51v0SR8z0POoNfktqEO6MaGig4YS8mosA@mail.gmail.com>
 <E6F4258A-B661-4F2C-8A35-278C9436930A@enterprisedb.com>
 <CAFiTN-s-VX3DLfqv7OOo0zGCTOZ3xVU9UZ6GgzBZdJTPNnYFNg@mail.gmail.com>
 <885D2EC9-7D06-4903-A17D-72C184A1E1C5@enterprisedb.com>
 <CAFiTN-uYg_PiZ854=5R-D9skratapAHKhaDAG7fhJv-Vn5UhaQ@mail.gmail.com>
 <5CC85F28-F210-47AC-8DA8-EC4E82AF3FD5@enterprisedb.com>
 <CAFiTN-sP5iFJGERTq4a4OLTCzjZKhLC5P6_k2bxQKXnuvdmzGA@mail.gmail.com>
 <CAFiTN-tTg0R5AHvXC1ro9XphroX1wUtc5t91hU_+7DeGTpE5VQ@mail.gmail.com>
 <0C0349EB-DD85-4DA8-82C7-DF70AB714592@enterprisedb.com>
 <CAFiTN-uD3WSeggU20yU6voGou6BDb-cQcfkE8B1rAzWKZVK-Bw@mail.gmail.com>
 <BA6E870F-241C-4DDB-B92D-DB39E679C7D4@enterprisedb.com>
 <CA+TgmobQUdfM2Q7WAR606RR2YehCZ5GHRKBc-VWfRj-1ZUKqDA@mail.gmail.com>
 <C4A3E5AE-CBB2-4E3F-8BAE-29EE216762C2@enterprisedb.com>
 <CAAJ_b96_Xr2a+D0j8xKbOnCzURcVQm0V2DS+T_pfc+wt3BpL8g@mail.gmail.com>
 <9DF93B83-FE46-4EC4-A99D-77A60729FD2D@enterprisedb.com>
 <CA+TgmoZT3kCOaKq6=m5wHGKh_9XQDbCC1LZXL6w_+NMB-n1+mw@mail.gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Precedence: bulk


> On Jul 30, 2020, at 10:59 AM, Robert Haas <robertmhaas@gmail.com> =
wrote:
>=20
> + curchunk =3D DatumGetInt32(fastgetattr(toasttup, 2,
> + ctx->toast_rel->rd_att, &isnull));
>=20
> Should we be worrying about the possibility of fastgetattr crapping
> out if the TOAST tuple is corrupted?

I think we should, but I'm not sure we should be worrying about it at =
this location.  If the toast index is corrupt, systable_getnext_ordered =
could trip over the index corruption in the process of retrieving the =
toast tuple, so checking the toast tuple only helps if the toast index =
does not cause a crash first.  I think the toast index should be checked =
before this point, ala verify_nbtree, so that we don't need to worry =
about that here.  It might also make more sense to verify the toast =
table ala verify_heapam prior to here, so we don't have to worry about =
that here either.  But that raises questions about whose responsibility =
this all is.  If verify_heapam checks the toast table and toast index =
before the main table, that takes care of it, but makes a mess of the =
idea of verify_heapam taking a start and end block, since verifying the =
toast index is an all or nothing proposition, not something to be done =
in incremental pieces.  If we leave verify_heapam as it is, then it is =
up to the caller to check the toast before the main relation, which is =
more flexible, but is more complicated and requires the user to remember =
to do it.  We could split the difference by having verify_heapam do =
nothing about toast, leaving it up to the caller, but make pg_amcheck =
handle it by default, making it easier for users to not think about the =
issue.  Users who want to do incremental checking could still keep track =
of the chunks that have already been checked, not just for the main =
relation, but for the toast relation, too, and give start and end block =
arguments to verify_heapam for the toast table check and then again for =
the main table check.  That doesn't fix the question of incrementally =
checking the index, though.

Looking at it a slightly different way, I think what is being checked at =
the point in the code you mention is the logical structure of the =
toasted value related to the current main table tuple, not the lower =
level tuple structure of the toast table.  We already have a function =
for checking a heap, namely verify_heapam, and we (or the caller, =
really) should be using that.  The clean way to do things is

	verify_heapam(toast_rel)
	verify_btreeam(toast_idx)
	verify_heapam(main_rel)

and then depending on how fast and loose you want to be, you can use the =
start and end block arguments, which are inherently a bit half-baked, =
given the lack of any way to be sure you check precisely the right range =
of blocks, and also you can be fast and loose about skipping the index =
check or not, as you see fit.

Thoughts?

=E2=80=94
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company