Re: pg_amcheck contrib application

On Wed, Mar 31, 2021 at 12:34 AM Mark Dilger
<mark(dot)dilger(at)enterprisedb(dot)com> wrote:
> These changes got lost between v11 and v12. I've put them back, as well as updating to use your language.

Here's an updated patch that includes your 0001 and 0002 plus a bunch
of changes by me. I intend to commit this version unless somebody
spots a problem with it.

Here are the things I changed:

- I renamed tuple_can_be_pruned to tuple_could_be_pruned because I
think it does a better job suggesting that we're uncertain about what
will happen.

- I got rid of bool header_garbled and changed it to bool result,
inverting the sense, because it didn't seem useful to have a function
that ended with if (some_boolean) return false; return true when I
could end the function with return some_other_boolean.

- I removed all the one-word comments that said /* corrupt */ or /*
checkable */ because they seemed redundant.

- In the xmin_status section of check_tuple_visibility(), I got rid of
the xmin_status == XID_IS_CURRENT_XID and xmin_status ==
XID_IN_PROGRESS cases because they were redundant with the xmin_status
!= XID_COMMITTED case.

- If xmax is a multi but seems to be garbled, I changed it to return
true rather than false. The inserter is known to have committed by
that point, so I think it's OK to try to deform the tuple. We just
shouldn't try to check TOAST.

- I changed both switches over xmax_status to break in each case and
then return true after instead of returning true for each case. I
think this is clearer.

- I changed get_xid_status() to perform the TransactionIdIs... checks
in the same order that HeapTupleSatisfies...() does them. I believe
that it's incorrect to conclude that the transaction must be in
progress because it neither IsCurrentTransaction nor DidCommit nor
DidAbort, because all of those things will be false for a transaction
that is running at the time of a system crash. The correct rule is
that if it neither IsCurrentTransaction nor IsInProgress nor DidCommit
then it's aborted.

- I moved a few comments and rewrote some others, including some of
the ones that you took from my earlier draft patch. The thing is, the
comment needs to be adjusted based on where you put it. Like, I had a
comment that says"It should be impossible for xvac to still be
running, since we've removed all that code, but even if it were, it
ought to be safe to read the tuple, since the original inserter must
have committed. But, if the xvac transaction committed, this tuple
(and its associated TOAST tuples) could be pruned at any time." which
in my version was right before a TransactionIdDidCommit() test, and
explains why that test is there and why the code does what it does as
a result. But in your version you've moved it to a place where we've
already tested that the transaction has committed, and more
importantly, where we've already tested that it's not still running.
Saying that it "should" be impossible for it not to be running when
we've *actually checked that* doesn't make nearly as much sense as it
does when we haven't checked that and aren't going to do so.

--
Robert Haas
EDB: http://www.enterprisedb.com