From 18b00da2e9c918baa5b970c755088e409f70e709 Mon Sep 17 00:00:00 2001 From: Mark Dilger Date: Tue, 16 Mar 2021 12:32:07 -0700 Subject: [PATCH v9 1/2] Fixing amcheck tuple visibility rules amcheck was considering a tuple as visible when it should have considered it dead, leading to checking of dead tuples and complaints when their toast was missing from the toast table, and perhaps to other problems. Extend amcheck's regression tests with a test case that reliably reproduces the buggy behavior fixed in this commit, to be sure it does not come back. --- contrib/amcheck/t/001_verify_heapam.pl | 13 +++- contrib/amcheck/verify_heapam.c | 93 ++++++++++++-------------- 2 files changed, 56 insertions(+), 50 deletions(-) diff --git a/contrib/amcheck/t/001_verify_heapam.pl b/contrib/amcheck/t/001_verify_heapam.pl index 6050feb712..b6fc640a53 100644 --- a/contrib/amcheck/t/001_verify_heapam.pl +++ b/contrib/amcheck/t/001_verify_heapam.pl @@ -4,7 +4,7 @@ use warnings; use PostgresNode; use TestLib; -use Test::More tests => 80; +use Test::More tests => 128; my ($node, $result); @@ -17,6 +17,17 @@ $node->append_conf('postgresql.conf', 'autovacuum=off'); $node->start; $node->safe_psql('postgres', q(CREATE EXTENSION amcheck)); +# +# Check for false positives against pg_statistic. There was a bug in the +# visibility checking logic that resulted in a consistently reproducible +# complaint about missing toast table entries for table pg_statistic. The +# problem was that main table entries were being checked despite being dead, +# which is wrong, and though the main table entries were not corrupt, the +# missing toast was reported. +# +$node->safe_psql('postgres', q(ANALYZE)); +check_all_options_uncorrupted('pg_catalog.pg_statistic', 'plain'); + # # Check a table with data loaded but no corruption, freezing, etc. # diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c index e614c12a14..dc57fe5774 100644 --- a/contrib/amcheck/verify_heapam.c +++ b/contrib/amcheck/verify_heapam.c @@ -758,58 +758,53 @@ check_tuple_header_and_visibilty(HeapTupleHeader tuphdr, HeapCheckContext *ctx) if (!(infomask & HEAP_XMAX_INVALID) && !HEAP_XMAX_IS_LOCKED_ONLY(infomask)) { - if (infomask & HEAP_XMAX_IS_MULTI) - { - XidCommitStatus status; - TransactionId xmax = HeapTupleGetUpdateXid(tuphdr); + XidCommitStatus status; + TransactionId xmax; - switch (get_xid_status(xmax, ctx, &status)) - { - /* not LOCKED_ONLY, so it has to have an xmax */ - case XID_INVALID: - report_corruption(ctx, - pstrdup("xmax is invalid")); - return false; /* corrupt */ - case XID_IN_FUTURE: - report_corruption(ctx, - psprintf("xmax %u equals or exceeds next valid transaction ID %u:%u", - xmax, - EpochFromFullTransactionId(ctx->next_fxid), - XidFromFullTransactionId(ctx->next_fxid))); - return false; /* corrupt */ - case XID_PRECEDES_RELMIN: - report_corruption(ctx, - psprintf("xmax %u precedes relation freeze threshold %u:%u", - xmax, - EpochFromFullTransactionId(ctx->relfrozenfxid), - XidFromFullTransactionId(ctx->relfrozenfxid))); - return false; /* corrupt */ - case XID_PRECEDES_CLUSTERMIN: - report_corruption(ctx, - psprintf("xmax %u precedes oldest valid transaction ID %u:%u", - xmax, - EpochFromFullTransactionId(ctx->oldest_fxid), - XidFromFullTransactionId(ctx->oldest_fxid))); - return false; /* corrupt */ - case XID_BOUNDS_OK: - switch (status) - { - case XID_IN_PROGRESS: - return true; /* HEAPTUPLE_DELETE_IN_PROGRESS */ - case XID_COMMITTED: - case XID_ABORTED: - return false; /* HEAPTUPLE_RECENTLY_DEAD or - * HEAPTUPLE_DEAD */ - } - } + if (infomask & HEAP_XMAX_IS_MULTI) + xmax = HeapTupleGetUpdateXid(tuphdr); + else + xmax = HeapTupleHeaderGetRawXmax(tuphdr); - /* Ok, the tuple is live */ + switch (get_xid_status(xmax, ctx, &status)) + { + /* not LOCKED_ONLY, so it has to have an xmax */ + case XID_INVALID: + report_corruption(ctx, + pstrdup("xmax is invalid")); + return false; /* corrupt */ + case XID_IN_FUTURE: + report_corruption(ctx, + psprintf("xmax %u equals or exceeds next valid transaction ID %u:%u", + xmax, + EpochFromFullTransactionId(ctx->next_fxid), + XidFromFullTransactionId(ctx->next_fxid))); + return false; /* corrupt */ + case XID_PRECEDES_RELMIN: + report_corruption(ctx, + psprintf("xmax %u precedes relation freeze threshold %u:%u", + xmax, + EpochFromFullTransactionId(ctx->relfrozenfxid), + XidFromFullTransactionId(ctx->relfrozenfxid))); + return false; /* corrupt */ + case XID_PRECEDES_CLUSTERMIN: + report_corruption(ctx, + psprintf("xmax %u precedes oldest valid transaction ID %u:%u", + xmax, + EpochFromFullTransactionId(ctx->oldest_fxid), + XidFromFullTransactionId(ctx->oldest_fxid))); + return false; /* corrupt */ + case XID_BOUNDS_OK: + switch (status) + { + case XID_IN_PROGRESS: + return true; /* HEAPTUPLE_DELETE_IN_PROGRESS */ + case XID_COMMITTED: + case XID_ABORTED: + return false; /* HEAPTUPLE_RECENTLY_DEAD or + * HEAPTUPLE_DEAD */ + } } - else if (!(infomask & HEAP_XMAX_COMMITTED)) - return true; /* HEAPTUPLE_DELETE_IN_PROGRESS or - * HEAPTUPLE_LIVE */ - else - return false; /* HEAPTUPLE_RECENTLY_DEAD or HEAPTUPLE_DEAD */ } return true; /* not dead */ } -- 2.21.1 (Apple Git-122.3)