From 86df30384a95bd2639d19282759c4cb09605dd73 Mon Sep 17 00:00:00 2001 From: Andrey Borodin Date: Sun, 4 Oct 2026 18:06:26 +0500 Subject: [PATCH v1 1/2] Detect incorrect HEAP_XMIN_INVALID hints in verify_heapam() A committed tuple marked with HEAP_XMIN_INVALID becomes invisible to heap scans, but verify_heapam() silently skips it. Report the contradiction with the transaction status that we already check. Exclude old-style VACUUM FULL tuples, whose hint can reflect the status of xvac rather than xmin. Distinguish known transaction statuses from commits assumed after clog truncation, including in the status cache. Only a known status can prove the hint bit wrong. Add coverage for incorrect and legitimate invalid-xmin hints. Discussion: https://postgr.es/m/CAH2-WzmHVeYY%3Dpjz9x8DhhxVjXHX0pvoQ-MdiB1Tt6%3Do2GTiKg%40mail.gmail.com --- contrib/amcheck/verify_heapam.c | 55 ++++++++++++++++++---- src/bin/pg_amcheck/t/004_verify_heapam.pl | 57 ++++++++++++++++++++++- 2 files changed, 103 insertions(+), 9 deletions(-) diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c index 46942b3580e..6270f2d21fc 100644 --- a/contrib/amcheck/verify_heapam.c +++ b/contrib/amcheck/verify_heapam.c @@ -112,6 +112,7 @@ typedef struct HeapCheckContext */ TransactionId cached_xid; XidCommitStatus cached_status; + bool cached_status_known; /* Values concerning the heap relation being checked */ Relation rel; @@ -216,7 +217,8 @@ static XidBoundsViolation check_mxid_valid_in_rel(MultiXactId mxid, HeapCheckContext *ctx); static XidBoundsViolation get_xid_status(TransactionId xid, HeapCheckContext *ctx, - XidCommitStatus *status); + XidCommitStatus *status, + bool *status_known); /* * Scan and report corruption in heap pages, optionally reconciling toasted @@ -1140,6 +1142,7 @@ check_tuple_visibility(HeapCheckContext *ctx, bool *xmin_commit_status_ok, XidCommitStatus xmin_status; XidCommitStatus xvac_status; XidCommitStatus xmax_status; + bool xmin_status_known; HeapTupleHeader tuphdr = ctx->tuphdr; ctx->tuple_could_be_pruned = true; /* have not yet proven otherwise */ @@ -1147,7 +1150,7 @@ check_tuple_visibility(HeapCheckContext *ctx, bool *xmin_commit_status_ok, /* If xmin is normal, it should be within valid range */ xmin = HeapTupleHeaderGetXmin(tuphdr); - switch (get_xid_status(xmin, ctx, &xmin_status)) + switch (get_xid_status(xmin, ctx, &xmin_status, &xmin_status_known)) { case XID_INVALID: /* Could be the result of a speculative insertion that aborted. */ @@ -1185,13 +1188,28 @@ check_tuple_visibility(HeapCheckContext *ctx, bool *xmin_commit_status_ok, if (!HeapTupleHeaderXminCommitted(tuphdr)) { if (HeapTupleHeaderXminInvalid(tuphdr)) - return false; /* inserter aborted, don't check */ + { + /* + * A committed inserter must not be hinted aborted. The hint can + * legitimately be set by old-style VACUUM FULL, however, based on + * the status of xvac rather than xmin. + * + * After clog truncation, an assumed commit status is not evidence + * of a bad hint bit; see get_xid_status(). + */ + if (xmin_status == XID_COMMITTED && xmin_status_known && + !(tuphdr->t_infomask & HEAP_MOVED)) + report_corruption(ctx, + psprintf("xmin %u is committed, but HEAP_XMIN_INVALID is set", + xmin)); + return false; /* don't check the tuple's contents */ + } /* Used by pre-9.0 binary upgrades */ else if (tuphdr->t_infomask & HEAP_MOVED_OFF) { xvac = HeapTupleHeaderGetXvac(tuphdr); - switch (get_xid_status(xvac, ctx, &xvac_status)) + switch (get_xid_status(xvac, ctx, &xvac_status, NULL)) { case XID_INVALID: report_corruption(ctx, @@ -1260,7 +1278,7 @@ check_tuple_visibility(HeapCheckContext *ctx, bool *xmin_commit_status_ok, { xvac = HeapTupleHeaderGetXvac(tuphdr); - switch (get_xid_status(xvac, ctx, &xvac_status)) + switch (get_xid_status(xvac, ctx, &xvac_status, NULL)) { case XID_INVALID: report_corruption(ctx, @@ -1420,7 +1438,7 @@ check_tuple_visibility(HeapCheckContext *ctx, bool *xmin_commit_status_ok, * this table. Now check the update xid from this multixact. */ xmax = HeapTupleGetUpdateXid(tuphdr); - switch (get_xid_status(xmax, ctx, &xmax_status)) + switch (get_xid_status(xmax, ctx, &xmax_status, NULL)) { case XID_INVALID: /* not LOCKED_ONLY, so it has to have an xmax */ @@ -1487,7 +1505,7 @@ check_tuple_visibility(HeapCheckContext *ctx, bool *xmin_commit_status_ok, /* xmax is an XID, not a MXID. Sanity check it. */ xmax = HeapTupleHeaderGetRawXmax(tuphdr); - switch (get_xid_status(xmax, ctx, &xmax_status)) + switch (get_xid_status(xmax, ctx, &xmax_status, NULL)) { case XID_INVALID: ctx->tuple_could_be_pruned = false; @@ -2172,13 +2190,21 @@ check_mxid_valid_in_rel(MultiXactId mxid, HeapCheckContext *ctx) * If the status argument is not NULL, and if and only if the transaction ID * appears to be valid in this relation, the status argument will be set with * the commit status of the transaction ID. + * + * When requesting a status, callers can also pass status_known to distinguish + * a determined status from a commit assumed after clog truncation. It is set + * to false for an assumed status or a bounds violation, and true otherwise. */ static XidBoundsViolation get_xid_status(TransactionId xid, HeapCheckContext *ctx, - XidCommitStatus *status) + XidCommitStatus *status, bool *status_known) { FullTransactionId fxid; FullTransactionId clog_horizon; + bool known = false; + + if (status_known != NULL) + *status_known = false; /* Quick check for special xids */ if (!TransactionIdIsValid(xid)) @@ -2187,6 +2213,8 @@ get_xid_status(TransactionId xid, HeapCheckContext *ctx, { if (status != NULL) *status = XID_COMMITTED; + if (status_known != NULL) + *status_known = true; return XID_BOUNDS_OK; } @@ -2218,9 +2246,16 @@ get_xid_status(TransactionId xid, HeapCheckContext *ctx, if (xid == ctx->cached_xid) { *status = ctx->cached_status; + if (status_known != NULL) + *status_known = ctx->cached_status_known; return XID_BOUNDS_OK; } + /* + * Hint bits can outlive their clog entries. An assumed commit status + * after truncation is sufficient for our visibility checks, but cannot + * prove a hint bit wrong. Keep that distinction with the cached status. + */ *status = XID_COMMITTED; LWLockAcquire(XactTruncationLock, LW_SHARED); clog_horizon = @@ -2228,6 +2263,7 @@ get_xid_status(TransactionId xid, HeapCheckContext *ctx, ctx); if (FullTransactionIdPrecedesOrEquals(clog_horizon, fxid)) { + known = true; if (TransactionIdIsCurrentTransactionId(xid)) *status = XID_IS_CURRENT_XID; else if (TransactionIdIsInProgress(xid)) @@ -2240,5 +2276,8 @@ get_xid_status(TransactionId xid, HeapCheckContext *ctx, LWLockRelease(XactTruncationLock); ctx->cached_xid = xid; ctx->cached_status = *status; + ctx->cached_status_known = known; + if (status_known != NULL) + *status_known = known; return XID_BOUNDS_OK; } diff --git a/src/bin/pg_amcheck/t/004_verify_heapam.pl b/src/bin/pg_amcheck/t/004_verify_heapam.pl index 95f1f34c90d..4e4a9f01691 100644 --- a/src/bin/pg_amcheck/t/004_verify_heapam.pl +++ b/src/bin/pg_amcheck/t/004_verify_heapam.pl @@ -224,7 +224,7 @@ my $relpath = "$pgdata/$rel"; # $ROWCOUNT is the total number of rows that we expect to insert into the page. # $ROWCOUNT_BASIC is the number of those rows that are related to basic # tuple validation, rather than update chain validation. -my $ROWCOUNT = 44; +my $ROWCOUNT = 49; my $ROWCOUNT_BASIC = 16; # First insert data needed for tests unrelated to update chain validation. @@ -316,6 +316,15 @@ my $in_progress_xid = $node->safe_psql( SELECT transaction FROM pg_prepared_xacts; )); +# Tuples for checking xmin hint bits, at offset numbers 45 through 49. +$node->safe_psql( + 'postgres', qq( + INSERT INTO public.test (a, b, c) + SELECT x'DEADF9F9DEADF9F9'::bigint, 'abcdefg', + repeat('w', 10000) + FROM generate_series(1, 5); + )); + my $relfrozenxid = $node->safe_psql('postgres', q(select relfrozenxid from pg_class where relname = 'test')); my $datfrozenxid = $node->safe_psql('postgres', @@ -405,6 +414,8 @@ use constant HEAP_XMAX_COMMITTED => 0x0400; use constant HEAP_XMAX_INVALID => 0x0800; use constant HEAP_NATTS_MASK => 0x07FF; use constant HEAP_XMAX_IS_MULTI => 0x1000; +use constant HEAP_MOVED_OFF => 0x4000; +use constant HEAP_MOVED_IN => 0x8000; use constant HEAP_KEYS_UPDATED => 0x2000; use constant HEAP_HOT_UPDATED => 0x4000; use constant HEAP_ONLY_TUPLE => 0x8000; @@ -735,6 +746,41 @@ for (my $tupidx = 0; $tupidx < $ROWCOUNT; $tupidx++) $tup->{t_xmin} = $in_progress_xid; $tup->{t_infomask} &= ~HEAP_XMIN_COMMITTED; } + elsif ($offnum >= 45 && $offnum <= 49) + { + $tup->{t_infomask} &= ~HEAP_XMIN_COMMITTED; + $tup->{t_infomask} |= HEAP_XMIN_INVALID; + + if ($offnum == 45) + { + # A committed inserter must not be hinted aborted. + my $xmin = $tup->{t_xmin}; + push @expected, + qr/${header}xmin $xmin is committed, but HEAP_XMIN_INVALID is set/; + } + elsif ($offnum == 46) + { + # Old-style VACUUM FULL moved this tuple off and committed. + $tup->{t_infomask} |= HEAP_MOVED_OFF; + $tup->{t_field3} = $tup->{t_xmin}; + } + elsif ($offnum == 47) + { + # Old-style VACUUM FULL moved this tuple in, then aborted. + $tup->{t_infomask} |= HEAP_MOVED_IN; + $tup->{t_field3} = $aborted_xid; + } + elsif ($offnum == 48) + { + # The hint is valid when the inserting transaction aborted. + $tup->{t_xmin} = $aborted_xid; + } + else + { + # Aborting a speculative insertion invalidates xmin itself. + $tup->{t_xmin} = 0; + } + } else { # The tests for update chain validation end up creating a bunch of @@ -757,6 +803,15 @@ $node->start; $node->command_checks_all( [ 'pg_amcheck', '--no-dependent-indexes', '--port' => $port, 'postgres' ], 2, [@expected], [], 'Expected corruption message output'); + +# The other uses of HEAP_XMIN_INVALID above must not be reported as corrupt. +is( $node->safe_psql( + 'postgres', + q(SELECT offnum FROM verify_heapam('test', check_toast => false) + WHERE offnum >= 45 ORDER BY offnum)), + '45', + 'only the incorrect xmin hint bit is reported'); + $node->safe_psql( 'postgres', qq( COMMIT PREPARED 'in_progress_tx'; base-commit: 852fd5b86e1a2ac1139c716eb4c96c09c03a632e -- That's all, folks. May the source be with you.