Re: [HACKERS] A design for amcheck heapam verification

On Thu, Mar 29, 2018 at 2:28 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> I understand we are adding a check to verify heap against index and
> this will take longer than before. When it runs does it report
> progress of the run via pg_stat_activity, so we can monitor how long
> it will take?

My point to Pavan was that the checker function that uses a weaker
lock, bt_index_check(), imitates a CREATE INDEX CONCURRENTLY. However,
unlike CIC, it acquires an ASL, not a ShareUpdateExclusiveLock. I
believe that this is safe, because we only actually perform a process
that's similar to the first heap scan of a CIC, without the other CIC
steps. (The variant that uses a ShareLock, bt_index_parent_check(),
imitates a CREATE INDEX, so no difference in lock strength there.)

amcheck is still just a couple of SQL-callable functions, so the
closest thing that there is to progress monitoring is DEBUG traces,
which I've found to be informative in real-world situations. Of
course, only a well informed user can interpret that. No change there.

> Locking is also an important concern.
>
> If we need a ShareLock to run the extended check and the check runs
> for a long time, when would we decide to run that? This sounds like it
> will cause a production outage, so what are the pre-conditions that
> would lead us to say "we'd better run this". For example, if running
> this is known to be signficantly faster than running CREATE INDEX,
> that might be an argument for someone to run this first if index
> corruption is suspected.

I think that most people will choose to use bt_index_check(), since,
as I said, it only requires an ASL, as opposed to
bt_index_parent_check(), which requires a ShareLock. This is
especially likely because there isn't much difference between how
thorough verification is. The fact that bt_index_check() is likely the
best thing to use for routine verification is documented in the
v10/master version [1], which hasn't changed. That said, there are
definitely environments where nobody cares a jot that a ShareLock is
needed, which is why we have bt_index_parent_check(). Often, amcheck
is used when the damage is already done, so that it can be fully
assessed.

This patch does not really change the interface; it just adds a new
heapallindexed argument, which has a default of 'false'.
bt_index_check() is unlikely to cause too many problems in production.
Heroku ran it on all databases at one point, and it wasn't too much
trouble. Of course, that was a version that lacked this heapallindexed
enhancement, which slows down verification by rather a lot when
actually used. My rough estimate is that heapallindexed verification
makes the process take 5x longer.

> If it detects an issue, can it fix the issue for the index by
> injecting correct entries? If not then we will have to run CREATE
> INDEX afterwards anyway, which makes it more likely that people would
> just run CREATE INDEX and not bother with the check.

It does not fix anything. I think that the new check is far more
likely to find problems in the heap than in the index, which is the
main reason for this.

The new check can only begin to run at the point where the index
structure has been found to be self-consistent, which is the main
reason why it seems like more of a heap checker than an index checker.
Also, that's what it actually found in the couple of interesting cases
that we've seen. It detects "freeze the dead" corruption, at least
with the test cases we have available. It also detects corruption
caused by failing to detect broken HOT chains during an initial CREATE
INDEX; there were two such bugs in CREATE INDEX CONCURRENTLY, one in
2012, and another in 2017, as you'll recall. The 2017 one (the CIC bug
that Pavan found through mechanical testing) went undetected for a
very long time; I think that a tool like this greatly increases our
odds of early detection of that kind of thing.

These are both issues that kind of seem like index corruption, that
are actually better understood as heap corruption. It's subtle.

> So my initial questions are about when we would run this and making
> sure that is documented.

Good question. I find it hard to offer general advice about this, to
be honest. In theory, you should never need to run it, because
everything should work, and in practice that's generally almost true.
I've certainly used it when investigating problems after the fact, and
as a general smoke-test, where it works well. I would perhaps
recommend running it once a week, although that's a fairly arbitrary
choice. The docs in v10 don't take a position on this, so while I tend
to agree we could do better, it is a preexisting issue.

[1] https://www.postgresql.org/docs/10/static/amcheck.html#id-1.11.7.11.7
--
Peter Geoghegan