From acee08646354ddb001d7b31a1b8932237bd40405 Mon Sep 17 00:00:00 2001 From: Mark Dilger Date: Wed, 24 Mar 2021 18:18:56 -0700 Subject: [PATCH v13 1/4] Refactoring function check_tuple_header_and_visibility Extending enum XidCommitStatus to include XID_IS_CURRENT_XID. The visibility code for verify_heapam() was conflating XID_IN_PROGRESS and XID_IS_CURRENT_XID under just one enum, making it harder to compare the logic to that used by vacuum's visibility function, which treats those two cases separately. Simplifying check_tuple_header_and_visibilty signature. It was taking both tuphdr and ctx arguments, but the tuphdr is just ctx->tuphdr, so it is a bit absurd to pass two arguments for this. Splitting check_tuple_header_and_visibilty() into two functions. check_tuple_header() and check_tuple_visibility() are split out as separate functions, but otherwise behave exactly as before. --- contrib/amcheck/verify_heapam.c | 82 +++++++++++++++++++-------------- 1 file changed, 47 insertions(+), 35 deletions(-) diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c index 6f972e630a..9172b5fd81 100644 --- a/contrib/amcheck/verify_heapam.c +++ b/contrib/amcheck/verify_heapam.c @@ -46,6 +46,7 @@ typedef enum XidBoundsViolation typedef enum XidCommitStatus { XID_COMMITTED, + XID_IS_CURRENT_XID, XID_IN_PROGRESS, XID_ABORTED } XidCommitStatus; @@ -133,8 +134,8 @@ static void check_tuple(HeapCheckContext *ctx); static void check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx); static bool check_tuple_attribute(HeapCheckContext *ctx); -static bool check_tuple_header_and_visibilty(HeapTupleHeader tuphdr, - HeapCheckContext *ctx); +static bool check_tuple_header(HeapCheckContext *ctx); +static bool check_tuple_visibility(HeapCheckContext *ctx); static void report_corruption(HeapCheckContext *ctx, char *msg); static TupleDesc verify_heapam_tupdesc(void); @@ -555,16 +556,11 @@ verify_heapam_tupdesc(void) } /* - * Check for tuple header corruption and tuple visibility. - * - * Since we do not hold a snapshot, tuple visibility is not a question of - * whether we should be able to see the tuple relative to any particular - * snapshot, but rather a question of whether it is safe and reasonable to - * check the tuple attributes. + * Check for tuple header corruption. * * Some kinds of corruption make it unsafe to check the tuple attributes, for * example when the line pointer refers to a range of bytes outside the page. - * In such cases, we return false (not visible) after recording appropriate + * In such cases, we return false (not checkable) after recording appropriate * corruption messages. * * Some other kinds of tuple header corruption confuse the question of where @@ -576,27 +572,16 @@ verify_heapam_tupdesc(void) * * Other kinds of tuple header corruption do not bear on the question of * whether the tuple attributes can be checked, so we record corruption - * messages for them but do not base our visibility determination on them. (In - * other words, we do not return false merely because we detected them.) - * - * For visibility determination not specifically related to corruption, what we - * want to know is if a tuple is potentially visible to any running - * transaction. If you are tempted to replace this function's visibility logic - * with a call to another visibility checking function, keep in mind that this - * function does not update hint bits, as it seems imprudent to write hint bits - * (or anything at all) to a table during a corruption check. Nor does this - * function bother classifying tuple visibility beyond a boolean visible vs. - * not visible. + * messages for them but we do not return false merely because we detected + * them. * - * The caller should already have checked that xmin and xmax are not out of - * bounds for the relation. - * - * Returns whether the tuple is both visible and sufficiently sensible to - * undergo attribute checks. + * Returns whether the tuple is sufficiently sensible to undergo visibility and + * attribute checks. */ static bool -check_tuple_header_and_visibilty(HeapTupleHeader tuphdr, HeapCheckContext *ctx) +check_tuple_header(HeapCheckContext *ctx) { + HeapTupleHeader tuphdr = ctx->tuphdr; uint16 infomask = tuphdr->t_infomask; bool header_garbled = false; unsigned expected_hoff; @@ -651,13 +636,34 @@ check_tuple_header_and_visibilty(HeapTupleHeader tuphdr, HeapCheckContext *ctx) if (header_garbled) return false; /* checking of this tuple should not continue */ - /* - * Ok, we can examine the header for tuple visibility purposes, though we - * still need to be careful about a few remaining types of header - * corruption. This logic roughly follows that of - * HeapTupleSatisfiesVacuum. Where possible the comments indicate which - * HTSV_Result we think that function might return for this tuple. - */ + return true; /* header ok */ +} + +/* + * Checks whether a tuple is visible for checking. + * + * Since we do not hold a snapshot, tuple visibility is not a question of + * whether we should be able to see the tuple relative to any particular + * snapshot, but rather a question of whether it is safe and reasonable to + * check the tuple attributes. + * + * For visibility determination not specifically related to corruption, what we + * want to know is if a tuple is potentially visible to any running + * transaction. If you are tempted to replace this function's visibility logic + * with a call to another visibility checking function, keep in mind that this + * function does not update hint bits, as it seems imprudent to write hint bits + * (or anything at all) to a table during a corruption check. Nor does this + * function bother classifying tuple visibility beyond a boolean visible vs. + * not visible. + * + * Returns whether the tuple is visible for checking. + */ +static bool +check_tuple_visibility(HeapCheckContext *ctx) +{ + HeapTupleHeader tuphdr = ctx->tuphdr; + uint16 infomask = tuphdr->t_infomask; + if (!HeapTupleHeaderXminCommitted(tuphdr)) { TransactionId raw_xmin = HeapTupleHeaderGetRawXmin(tuphdr); @@ -704,6 +710,7 @@ check_tuple_header_and_visibilty(HeapTupleHeader tuphdr, HeapCheckContext *ctx) switch (status) { case XID_IN_PROGRESS: + case XID_IS_CURRENT_XID: return true; /* HEAPTUPLE_DELETE_IN_PROGRESS */ case XID_COMMITTED: case XID_ABORTED: @@ -748,6 +755,7 @@ check_tuple_header_and_visibilty(HeapTupleHeader tuphdr, HeapCheckContext *ctx) case XID_COMMITTED: break; case XID_IN_PROGRESS: + case XID_IS_CURRENT_XID: return true; /* insert or delete in progress */ case XID_ABORTED: return false; /* HEAPTUPLE_DEAD */ @@ -795,6 +803,7 @@ check_tuple_header_and_visibilty(HeapTupleHeader tuphdr, HeapCheckContext *ctx) switch (status) { case XID_IN_PROGRESS: + case XID_IS_CURRENT_XID: return true; /* HEAPTUPLE_DELETE_IN_PROGRESS */ case XID_COMMITTED: case XID_ABORTED: @@ -1247,7 +1256,10 @@ check_tuple(HeapCheckContext *ctx) * corrupt to continue checking, or if the tuple is not visible to anyone, * we cannot continue with other checks. */ - if (!check_tuple_header_and_visibilty(ctx->tuphdr, ctx)) + if (!check_tuple_header(ctx)) + return; + + if (!check_tuple_visibility(ctx)) return; /* @@ -1448,7 +1460,7 @@ get_xid_status(TransactionId xid, HeapCheckContext *ctx, if (FullTransactionIdPrecedesOrEquals(clog_horizon, fxid)) { if (TransactionIdIsCurrentTransactionId(xid)) - *status = XID_IN_PROGRESS; + *status = XID_IS_CURRENT_XID; else if (TransactionIdDidCommit(xid)) *status = XID_COMMITTED; else if (TransactionIdDidAbort(xid)) -- 2.21.1 (Apple Git-122.3)