From cecac15bae8598f826103c25dd8036d78fdfa07c Mon Sep 17 00:00:00 2001 From: Andrey Borodin Date: Tue, 6 Oct 2026 14:51:21 +0500 Subject: [PATCH v2 1/2] Detect incorrect invalid transaction hints in verify_heapam() The snapshot-export bug can leave tuples with an invalid xmin or xmax hint for a committed transaction. verify_heapam() currently trusts those hints and can miss this damage. Check invalid hints against known commit status, including a multixact's updater. Exclude finished lockers and old VACUUM FULL tuples where invalid hints are legitimate, and do not use a commit assumed after clog truncation as evidence. Add coverage for invalid hints and their valid exceptions. Discussion: https://postgr.es/m/B993F6CC-03D0-440F-9D7C-FEF65D19C082@yandex-team.ru --- contrib/amcheck/verify_heapam.c | 78 +++++++++-- src/bin/pg_amcheck/t/004_verify_heapam.pl | 151 +++++++++++++++++++++- 2 files changed, 218 insertions(+), 11 deletions(-) diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c index 46942b3580e..89a4d80935f 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,8 @@ check_tuple_visibility(HeapCheckContext *ctx, bool *xmin_commit_status_ok, XidCommitStatus xmin_status; XidCommitStatus xvac_status; XidCommitStatus xmax_status; + bool xmin_status_known; + bool xmax_status_known; HeapTupleHeader tuphdr = ctx->tuphdr; ctx->tuple_could_be_pruned = true; /* have not yet proven otherwise */ @@ -1147,7 +1151,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 +1189,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 marked invalid", + 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 +1279,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, @@ -1392,6 +1411,28 @@ check_tuple_visibility(HeapCheckContext *ctx, bool *xmin_commit_status_ok, if (tuphdr->t_infomask & HEAP_XMAX_INVALID) { + /* + * A finished locker can legitimately be hinted invalid even if it + * committed, but a committed updater cannot. For a multixact, check + * its updater, not the multixact ID or its lockers. + */ + if (!HEAP_XMAX_IS_LOCKED_ONLY(tuphdr->t_infomask)) + { + xmax = (tuphdr->t_infomask & HEAP_XMAX_IS_MULTI) ? + HeapTupleGetUpdateXid(tuphdr) : HeapTupleHeaderGetRawXmax(tuphdr); + if (TransactionIdIsNormal(xmax) && + get_xid_status(xmax, ctx, &xmax_status, &xmax_status_known) == XID_BOUNDS_OK && + xmax_status == XID_COMMITTED && xmax_status_known) + { + report_corruption(ctx, + psprintf((tuphdr->t_infomask & HEAP_XMAX_IS_MULTI) ? + "update xid %u is committed, but marked invalid" : + "xmax %u is committed, but marked invalid", + xmax)); + return true; /* tuple may be dead; don't check its TOAST */ + } + } + /* * This tuple is live. A concurrently running transaction could * delete it before we get around to checking the toast, but any such @@ -1420,7 +1461,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 +1528,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 +2213,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 +2236,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 +2269,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 +2286,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 +2299,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..9c282cd9603 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 = 57; my $ROWCOUNT_BASIC = 16; # First insert data needed for tests unrelated to update chain validation. @@ -316,6 +316,49 @@ my $in_progress_xid = $node->safe_psql( SELECT transaction FROM pg_prepared_xacts; )); +# Tuples for checking hint bits, at offset numbers 45 through 57. +$node->safe_psql( + 'postgres', qq( + INSERT INTO public.test (a, b, c) + SELECT x'DEADF9F9DEADF9F9'::bigint, 'abcdefg', + repeat('w', 10000) + FROM generate_series(1, 13); + )); + +# Make real multixacts with a committed updater, an aborted updater, and +# lockers only. Keep the first locker open until all three have been made. +$node->safe_psql( + 'postgres', q( + CREATE TABLE multixact_test (id int PRIMARY KEY, val int); + INSERT INTO multixact_test VALUES (1, 0), (2, 0), (3, 0); + )); +my $locker = $node->background_psql('postgres'); +$locker->query_safe( + q(BEGIN; SELECT * FROM multixact_test FOR KEY SHARE;)); +$node->safe_psql( + 'postgres', q( + UPDATE multixact_test SET val = 1 WHERE id = 1; + )); +$node->safe_psql( + 'postgres', q( + BEGIN; + UPDATE multixact_test SET val = 1 WHERE id = 2; + ROLLBACK; + )); +$node->safe_psql( + 'postgres', q( + SELECT * FROM multixact_test WHERE id = 3 FOR KEY SHARE; + )); +my @multixacts = split '\n', $node->safe_psql( + 'postgres', q( + SELECT t_xmax FROM heap_page_items(get_raw_page('multixact_test', 0)) + WHERE lp BETWEEN 1 AND 3 AND t_infomask & 4096 <> 0 + ORDER BY lp; + )); +scalar @multixacts == 3 or BAIL_OUT('expected three multixacts'); +$locker->query_safe('COMMIT;'); +$locker->quit; + my $relfrozenxid = $node->safe_psql('postgres', q(select relfrozenxid from pg_class where relname = 'test')); my $datfrozenxid = $node->safe_psql('postgres', @@ -398,6 +441,7 @@ $node->stop; # Some #define constants from access/htup_details.h for use while corrupting. use constant HEAP_HASNULL => 0x0001; +use constant HEAP_XMAX_KEYSHR_LOCK => 0x0010; use constant HEAP_XMAX_LOCK_ONLY => 0x0080; use constant HEAP_XMIN_COMMITTED => 0x0100; use constant HEAP_XMIN_INVALID => 0x0200; @@ -405,6 +449,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; @@ -591,10 +637,11 @@ for (my $tupidx = 0; $tupidx < $ROWCOUNT; $tupidx++) # Set both HEAP_XMAX_COMMITTED and HEAP_XMAX_IS_MULTI $tup->{t_infomask} |= HEAP_XMAX_COMMITTED; $tup->{t_infomask} |= HEAP_XMAX_IS_MULTI; - $tup->{t_xmax} = 4; + my $future_mxid = $multixacts[-1] + 1; + $tup->{t_xmax} = $future_mxid; push @expected, - qr/${header}multitransaction ID 4 equals or exceeds next valid multitransaction ID 1/; + qr/${header}multitransaction ID $future_mxid equals or exceeds next valid multitransaction ID \d+/; } elsif ($offnum == 15) { @@ -735,6 +782,88 @@ 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 marked invalid/; + } + 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; + } + } + elsif ($offnum >= 50 && $offnum <= 57) + { + # Offset 57 keeps this committed updater without any xmax hints. + $tup->{t_xmax} = $tup->{t_xmin}; + $tup->{t_infomask} &= ~(HEAP_XMAX_INVALID | HEAP_XMAX_COMMITTED); + + if ($offnum == 50) + { + # A committed updater must not be hidden by an invalid-xmax hint. + $tup->{t_infomask} |= HEAP_XMAX_INVALID; + my $xmax = $tup->{t_xmax}; + push @expected, + qr/${header}xmax $xmax is committed, but marked invalid/; + } + elsif ($offnum == 51 || $offnum == 52) + { + # A committed locker can have either xmax hint. HEAP_UPDATED + # describes how this version was created, not what xmax did. + $tup->{t_infomask} |= + HEAP_XMAX_LOCK_ONLY | HEAP_XMAX_KEYSHR_LOCK | HEAP_UPDATED; + $tup->{t_infomask} |= + $offnum == 51 ? HEAP_XMAX_INVALID : HEAP_XMAX_COMMITTED; + } + elsif ($offnum == 53) + { + # An aborted updater can legitimately be hinted invalid. + $tup->{t_xmax} = $aborted_xid; + $tup->{t_infomask} |= HEAP_XMAX_INVALID; + } + elsif ($offnum >= 54 && $offnum <= 56) + { + $tup->{t_xmax} = $multixacts[$offnum - 54]; + $tup->{t_infomask} |= HEAP_XMAX_IS_MULTI | HEAP_XMAX_INVALID; + if ($offnum == 54) + { + # Only the multixact with a committed updater contradicts + # HEAP_XMAX_INVALID. Its lockers' status is irrelevant. + push @expected, + qr/${header}update xid \d+ is committed, but marked invalid/; + } + elsif ($offnum == 56) + { + $tup->{t_infomask} |= + HEAP_XMAX_LOCK_ONLY | HEAP_XMAX_KEYSHR_LOCK; + } + } + } else { # The tests for update chain validation end up creating a bunch of @@ -757,6 +886,22 @@ $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 BETWEEN 45 AND 49 ORDER BY offnum)), + '45', + 'only the incorrect xmin hint bit is reported'); + +is( $node->safe_psql( + 'postgres', + q(SELECT offnum FROM verify_heapam('test', check_toast => false) + WHERE offnum BETWEEN 50 AND 57 ORDER BY offnum)), + "50\n54", + 'only the incorrect xmax hint bits are reported'); + $node->safe_psql( 'postgres', qq( COMMIT PREPARED 'in_progress_tx'; base-commit: e8f4f9e3ce2a7a17790972e04631aa999b72abfd -- That's all, folks. May the source be with you.