> On Nov 20, 2024, at 6:39 AM, Andrey M. Borodin <x4...@yandex-team.ru> wrote:
>
>
>
>> On 20 Nov 2024, at 15:58, Andrey M. Borodin <x4...@yandex-team.ru> wrote:
>>
>> PFA the patch doing so.
>
> Ugh. The patch is simply dysfunctional, sorry. xmax_status is being checked
> uninitiated.
> But, well, it highlights the idea: make verify_heapam() aware of such
> corruptions.
> What do you think?
I like the idea of increasing the corruption checking coverage. The worry with
these patches is that we'll overlook some legitimate use case of the status
bits and call it corruption when it isn't. Indeed, that appears to be the case
with this patch, assuming I initialized the xmax_status field in the way you
had in mind, and that applying it to REL_17_STABLE is ok. (Maybe this would
work differently on HEAD?)
+ get_xid_status(xmax, ctx, &xmax_status);
+ if (xmax_status == XID_COMMITTED && (tuphdr->t_infomask &
HEAP_UPDATED))
+ {
+ report_corruption(ctx,
+ psprintf("committed xmax %u while tuple
has HEAP_XMAX_INVALID and HEAP_UPDATED flags",
+ xmax));
+ }
That results in TAP test failures on a uncorrupted but frozen table:
# +++ tap check in contrib/amcheck +++
t/001_verify_heapam.pl ......... 74/?
# Failed test 'all-frozen not corrupted table'
# at t/001_verify_heapam.pl line 53.
# got: '30|117||committed xmax 2 while tuple has HEAP_XMAX_INVALID and
HEAP_UPDATED flags'
# expected: ''
t/001_verify_heapam.pl ......... 257/? # Looks like you failed 1 test of 272.
t/001_verify_heapam.pl ......... Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/272 subtests
The first part of your patch which checks the xmin_status seems ok at first
glance.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
