Hi all,
I think there might be an issue with the error messages in contrib/amcheck/verify_heapam.c.
The test comment in src/bin/pg_amcheck/t/004_verify_heapam.pl (line 715) clearly states:
```
# Tuple at offset 43 is the successor of this one
```
This indicates that for the test case at offnum == 36, the error message should report "offset 43" (the successor), not "offset 36" (the current tuple).
However, when I updated the test expectation from \d+ wildcard to the exact value offset 43, the test fails.
Patch 1: Updating the wildcard matching from "\d+" to the exact value "43" makes the test more precise and clear. After all, this change led to the discovery of the bug you fixed in Patch 2 - the original wildcard masked the problem. Since the test data was engineered to be deterministic and there is precedent for hardcoding test values, I think Patch 1 is a good addition.
Lines 283-296 implies test data was engineer to be deterministic.
# Data for HOT chain validation, so not calling VACUUM FREEZE.
$node->safe_psql(
'postgres', qq(
INSERT INTO public.test (a, b, c)
VALUES ( x'DEADF9F9DEADF9F9'::bigint, 'abcdefg',
generate_series(7,15)); -- offset numbers 28 to 36
UPDATE public.test SET c = 'a' WHERE c = '7'; -- offset number 37
UPDATE public.test SET c = 'a' WHERE c = '10'; -- offset number 38
UPDATE public.test SET c = 'a' WHERE c = '11'; -- offset number 39
UPDATE public.test SET c = 'a' WHERE c = '12'; -- offset number 40
UPDATE public.test SET c = 'a' WHERE c = '13'; -- offset number 41
UPDATE public.test SET c = 'a' WHERE c = '14'; -- offset number 42
UPDATE public.test SET c = 'a' WHERE c = '15'; -- offset number 43
));
I think there are a few other test cases that Patch 2 will affect but the diff will be masked out by regexp. It is up to you if you want to follow through with the exercise. At a minimum, I think we should address test offnum == 33 a line 693.
This makes me wonder whether the current code in verify_heapam.c (lines 777, 793, 799) should be using nextoffnum instead of ctx.offnum.
This would also be consistent with similar error messages at lines 746-747 and 753-754, which use nextoffnum when referring to the produced tuple location.
Great catch on Patch 2. Lines 633-664 show that ctx.offnum is simply an iteration of the line pointer in the page and nextoffnum maps to the successor of each iteration's line pointer.
Given that the error message is "... was updated to produce a tuple at offset %d ...", the successor's offset should be used. Therefore the change on lines 777, 793, and 799 from using ctx.offnum to using nextoffnum looks good.
Both patches look great to me.
