[PATCH] Align verify_heapam.c error message offset with test expectations

Hi zengman,

On Wed, Jan 21, 2026 at 10:37 PM zengman <zengman@halodbtech.com> wrote:

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.
This makes me wonder whether the current code in verify_heapam.c (lines
777, 793, 799) should be using nextoffnum instead of ctx.offnum.

Fully agreed.
I think there's no need to modify the expectations. Seems the 0002 patch
alone is sufficient.

Fully agreed.
I think there's no need to modify the expectations. Seems the 0002 patch alone is sufficient.

Hi,

Thanks for your feedback. I don’t have a strong opinion either way about whether to keep or remove the 0001 patch — I included it more as a way to illustrate the issue.

--
Regards,
Man Zeng
www.openhalo.org

On Thu, Jan 22, 2026 at 9:44 AM zengman <zengman@halodbtech.com> wrote:

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.

Both patches look great to me.

Hi,

I truly appreciate your approval, along with your thorough review and valuable insights.
That said, I don’t plan to make further revisions to Patch 1 at this stage—and I tend to think that Patch 2 might just be sufficient on its own.
Of course, this will ultimately depend on the committers.
Thanks again!

--
Regards,
Man Zeng
www.openhalo.org