> On Jul 30, 2020, at 10:59 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> + curchunk = DatumGetInt32(fastgetattr(toasttup, 2,
> + ctx->toast_rel->rd_att, &isnull));
>
> 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_orderedcould trip over the index corruption in the process of retrieving the toast tuple, so checking
thetoast tuple only helps if the toast index does not cause a crash first. I think the toast index should be checked
beforethis point, ala verify_nbtree, so that we don't need to worry about that here. It might also make more sense to
verifythe toast table ala verify_heapam prior to here, so we don't have to worry about that here either. But that
raisesquestions about whose responsibility this all is. If verify_heapam checks the toast table and toast index before
themain table, that takes care of it, but makes a mess of the idea of verify_heapam taking a start and end block, since
verifyingthe toast index is an all or nothing proposition, not something to be done in incremental pieces. If we leave
verify_heapamas it is, then it is up to the caller to check the toast before the main relation, which is more flexible,
butis more complicated and requires the user to remember to do it. We could split the difference by having
verify_heapamdo nothing about toast, leaving it up to the caller, but make pg_amcheck handle it by default, making it
easierfor users to not think about the issue. Users who want to do incremental checking could still keep track of the
chunksthat have already been checked, not just for the main relation, but for the toast relation, too, and give start
andend block arguments to verify_heapam for the toast table check and then again for the main table check. That
doesn'tfix 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
logicalstructure of the toasted value related to the current main table tuple, not the lower level tuple structure of
thetoast table. We already have a function for checking a heap, namely verify_heapam, and we (or the caller, really)
shouldbe 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
inherentlya bit half-baked, given the lack of any way to be sure you check precisely the right range of blocks, and
alsoyou can be fast and loose about skipping the index check or not, as you see fit.
Thoughts?
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
