Re: pg_amcheck contrib application

On Mon, Apr 26, 2021 at 1:52 PM Mark Dilger
<mark(dot)dilger(at)enterprisedb(dot)com> wrote:
> The attached patch changes amcheck corruption reports as discussed upthread. This patch is submitted for the v14 development cycle as a bug fix, per your complaint that the committed code generates reports sufficiently confusing to a user as to constitute a bug.
>
> All other code refactoring and additional checks discussed upthread are reserved for the v15 development cycle and are not included here.
>
> The minimal patch (not attached) that does not rename any variables is 135 lines. Your patch was 159 lines. The patch (attached) which includes your variable renaming is 174 lines.

Hi,

I have compared this against my version. I found the following differences:

1. This version passes last_chunk_seq rather than extsize to
check_toast_tuple(). But this results in having to call
VARATT_EXTERNAL_GET_EXTSIZE() inside that function. I thought it was
nicer to do that in the caller, so that we don't do it twice.

2. You fixed some out-of-date comments.

3. You move the test for an unexpected chunk sequence further down in
the function. I don't see the point; I had put it by the related null
check, and still think that's better. You also deleted my comment /*
Either the TOAST index is corrupt, or we don't have all chunks. */
which I would have preferred to keep.

4. You don't return if chunk_seq > last_chunk_seq. That seems wrong,
because we cannot compute a sensible expected size in that case. I
think your code will subtract a larger value from a smaller one and,
this being unsigned arithmetic, say that the expected chunk size is
something gigantic. Returning and not issuing that complaint at all
seems better.

5. You fixed the incorrect formula I had introduced for the expected
size of the last chunk.

6. You changed the variable name in check_toasted_attribute() from
expected_chunkno to chunkno, and initialized it later in the function
instead of at declaration time. I don't find this to be an
improvement; including the word "expected" seems to me to be
substantially clearer. But I think I should have gone with
expected_chunk_seq for better consistency.

7. You restored the message "toast value %u was expected to end at
chunk %d, but ended at chunk %d" which my version deleted. I deleted
that message because I thought it was redundant, but I guess it's not:
there's nothing else to complain if the sequence of chunks ends early.
I think we should change the test from != to < though, because if it's
>, then we must have already complained about unexpected chunks. Also,
I think the message is actually wrong, because even though you renamed
the variable, it still ends up being the expected next chunkno rather
than the last chunkno we actually saw.

PFA my counter-proposal based on the above analysis.

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