Subject: room for improvement in amcheck btree checking?

From: Mark Dilger <[email-masked]>
Date: Tue, 1 Dec 2020 12:39:02 -0800

Peter,

While working on the pg_amcheck code for [1], I discovered an unexpected deficiency in the way btree indexes are checked.  So far as I can tell from the docs [2], the deficiency does not violate any promises that amcheck is making, but I found it rather surprising all the same.  To reproduce:

1) Create a (possibly empty) table and btree index over the table.
2) Flush buffers and backup a copy of the heap relation file.
3) Load (more) data into the table.
4) Flushing buffers as needed, revert the heap relation file to the backup previously taken.
5) Run bt_index_check and bt_index_parent_check with and without heapallindexed and/or rootdescend.  Note that the index passes all checks.
6) Run a SQL query that uses a sequential scan on the table and observe no errors.
7) Run a SQL query that uses an index scan on the table and see that it errors with something like:

   ERROR:  could not read block 0 in file "base/13097/16391": read only 0 of 8192 bytes

I found it surprising that even when precisely zero of the tids in the index exist in the table the index checks all come back clean.  The heapallindexed check is technically running as advertised, checking that all of the zero tuples in the heap are present in the index.  That is a pretty useless check under this condition, though.  Is a "indexallheaped" option (by some less crazy name) needed?

Users might also run into this problem when a heap relation file gets erroneously shortened by some number of blocks but not fully truncated, or perhaps with torn page writes.

Have you already considered and rejected a "indexallheaped" type check?

Background
-------

I have up until recently been focused on corruption caused by twiddling the bits within heap and index relation pages, but real-world user error, file system error, and perhaps race conditions in the core postgresql code seem at least as likely to result in missing or incorrect versions of blocks of relation files rather than individual bytes within those blocks being wrong.  Per our discussions in [3], not all corruptions that can be created under laboratory conditions are equally likely to occur in the wild, and it may be reasonable to only harden the amcheck code against corruptions that are more likely to happen in actual practice.

To make it easier for tap tests to cover common corruption type scenarios, I have been extending PostgresNode.pm with functions to perform these kinds of file system corruptions.  I expect to post that work in another thread soon.  I am not embedding any knowledge of the internal structure of heap, index, or toast relations in PostgresNode, only creating functions to archive versions of files and perform full or partial reversions of them later.

The ultimate goal of this work is to have sufficient regression tests to demonstrate that pg_amcheck can be run with default options against a system corrupted in these common ways without crashing, and with reasonable likelihood of detecting these common corruptions.  Users might understand that hard to detect corruption will go unnoticed, but it would be harder to explain to them why, immediately after getting a clean bill of health on their system, a query bombed out with the sort of error shown above.

[1] https://commitfest.postgresql.org/31/2670/
[2] https://www.postgresql.org/docs/13/amcheck.html
[3] https://www.postgresql.org/message-id/flat/CAH2-WznaU6HcahLV4Hg-DnhEmW8DuSdYfn3vfWXoj3Me9jq%3DsQ%40mail.gmail.com#0691475da5e9163d21b13fc415095801

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 1 Dec 2020 20:26:53 -0300

On 2020-Dec-01, Mark Dilger wrote:

> I found it surprising that even when precisely zero of the tids in the
> index exist in the table the index checks all come back clean.

Yeah, I've seen this kind of symptom in production databases (indexes
pointing to non-existant heap pages).

I think one useful cross-check that amcheck could do, is verify that if
a heap page is referenced from the index, then the heap page must exist.
Otherwise, it's a future index corruption of sorts: the old index
entries will point to the wrong new heap tuples as soon as the table
grows again.

---

From: Peter Geoghegan <[email-masked]>
Date: Tue, 1 Dec 2020 16:38:16 -0800

> Users might also run into this problem when a heap relation file gets erroneously shortened by some number of blocks but not fully truncated, or perhaps with torn page writes.

It would probably be possible to detect this exact condition (though
not other variants of the more general problem) relatively easily.
Perhaps by doing something with RelationOpenSmgr() with the heap
relation, while applying a little knowledge of what must be true about
the heap relation given what is known about the index relation. (I'm
not 100% sure that that's possible, but offhand it seems like it
probably is.)

See also: commit 6754fe65a4c, which tightened things up in this area
for the index relation itself.

> Have you already considered and rejected a "indexallheaped" type check?

Actually, I pointed out that we should do something along these lines
very recently:

https://postgr.es/m/CAH2-Wz=[email-masked]

I'd be happy to see you pursue it.

> I have up until recently been focused on corruption caused by twiddling the bits within heap and index relation pages, but real-world user error, file system error, and perhaps race conditions in t…

Right. I also like to harden amcheck in ways that happen to be easy,
especially when it seems like it might kind of work as a broad
defense that doesn't consider any particular scenario. For example,
hardening to make sure the an index tuple's lp_len matches
IndexTupleSize() for the tuple proper (this also kind of works as
storage format documentation). I am concerned about both costs and
benefits.

FWIW, this is a perfect example of the kind of hardening that makes
sense to me. Clearly this kind of failure is a reasonably plausible
scenario (probably even a known real world scenario with catalog
corruption), and clearly we could do something about it that's pretty
simple and obviously low risk. It easily meets the standard that I
might apply here.

--
Peter Geoghegan

---

From: Mark Dilger <[email-masked]>
Date: Tue, 1 Dec 2020 18:03:48 -0800

> On Dec 1, 2020, at 4:38 PM, Peter Geoghegan <[email-masked]> wrote:
> 

Yes, some of the test code I've been playing with already hit the error messages introduced in that commit.

> https://postgr.es/m/CAH2-Wz=[email-masked]

Right.

> I'd be happy to see you pursue it.

I'm not sure how much longer I'll be pursuing corruption checkers before switching to another task.  Certainly, I'm interested in pursuing this if time permits.

> simple and obviously low risk. It easily meets the standard that I

Ok, it seems we're in general agreement about that.

—
Mark Dilger