amcheck: Generalize one of the recently-added update chain checks.

robertmhaas · robertmhaas · commit c87aff065c33 · 2023-03-27T13:37:16.000-04:00
Commit bbc1376 checked that if a redirected line pointer pointed to a tuple, the tuple should be marked both HEAP_ONLY_TUPLE and HEAP_UPDATED. But Andres Freund pointed out that *any* tuple that is marked HEAP_ONLY_TUPLE should be marked HEAP_UPDATED, not just one that is the target of a redirected line pointer. Do that instead. To see why this is better, consider a redirect line pointer A which points to a heap-only tuple B which points (via CTID) to another heap-only tuple C. With the old code, we'd complain if B was not marked HEAP_UPDATED, but with this change, we'll complain if either B or C is not marked HEAP_UPDATED. (Note that, with or without this commit, if either B or C were not marked HEAP_ONLY_TUPLE, we would also complain about that.) Discussion: http://postgr.es/m/CA%2BTgmobLypZx%3DcOH%2ByY1GZmCruaoucHm77A6y_-Bo%3Dh-_3H28g%40mail.gmail.com
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
@@ -624,17 +624,6 @@ verify_heapam(PG_FUNCTION_ARGS)
 											   (unsigned) nextoffnum));
 				}
 
-				/*
-				 * Redirects are created by updates, so successor should be
-				 * the result of an update.
-				 */
-				if ((next_htup->t_infomask & HEAP_UPDATED) == 0)
-				{
-					report_corruption(&ctx,
-									  psprintf("redirected line pointer points to a non-heap-updated tuple at offset %u",
-											   (unsigned) nextoffnum));
-				}
-
 				/* HOT chains should not intersect. */
 				if (predecessor[nextoffnum] != InvalidOffsetNumber)
 				{
@@ -954,6 +943,15 @@ check_tuple_header(HeapCheckContext *ctx)
 		 */
 	}
 
+	if (HeapTupleHeaderIsHeapOnly(tuphdr) &&
+		((tuphdr->t_infomask & HEAP_UPDATED) == 0))
+	{
+		report_corruption(ctx,
+						  psprintf("tuple is heap only, but not the result of an update"));
+
+		/* Here again, we can still perform further checks. */
+	}
+
 	if (infomask & HEAP_HASNULL)
 		expected_hoff = MAXALIGN(SizeofHeapTupleHeader + BITMAPLEN(ctx->natts));
 	else
diff --git a/src/bin/pg_amcheck/t/004_verify_heapam.pl b/src/bin/pg_amcheck/t/004_verify_heapam.pl
@@ -617,12 +617,10 @@ sub header
 	}
 	elsif ($offnum == 17)
 	{
-		# at offnum 19 we will unset HEAP_ONLY_TUPLE and HEAP_UPDATED flags.
+		# at offnum 19 we will unset HEAP_ONLY_TUPLE flag
 		die "offnum $offnum should be a redirect" if defined $tup;
 		push @expected,
 			qr/${header}redirected line pointer points to a non-heap-only tuple at offset \d+/;
-		push @expected,
-			qr/${header}redirected line pointer points to a non-heap-updated tuple at offset \d+/;
 	}
 	elsif ($offnum == 18)
 	{
@@ -637,10 +635,9 @@ sub header
 	}
 	elsif ($offnum == 19)
 	{
-		# unset HEAP_ONLY_TUPLE and HEAP_UPDATED flag, so that update chain
-		# validation will complain about offset 17
+		# unset HEAP_ONLY_TUPLE flag, so that update chain validation will
+		# complain about offset 17
 		$tup->{t_infomask2} &= ~HEAP_ONLY_TUPLE;
-		$tup->{t_infomask} &= ~HEAP_UPDATED;
 	}
 	elsif ($offnum == 22)
 	{
@@ -688,6 +685,8 @@ sub header
 		$tup->{t_infomask2} |= HEAP_ONLY_TUPLE;
 		push @expected,
 		  qr/${header}tuple is root of chain but is marked as heap-only tuple/;
+		push @expected,
+		  qr/${header}tuple is heap only, but not the result of an update/;
 	}
 	elsif ($offnum == 33)
 	{

Original file line number	Diff line number	Diff line change
`@@ -617,12 +617,10 @@ sub header`
`617`	`617`	`}`
`618`	`618`	`elsif ($offnum == 17)`
`619`	`619`	`{`
`620`		`- # at offnum 19 we will unset HEAP_ONLY_TUPLE and HEAP_UPDATED flags.`
	`620`	`+ # at offnum 19 we will unset HEAP_ONLY_TUPLE flag`
`621`	`621`	`die "offnum $offnum should be a redirect" if defined $tup;`
`622`	`622`	`push @expected,`
`623`	`623`	`qr/${header}redirected line pointer points to a non-heap-only tuple at offset \d+/;`
`624`		`- push @expected,`
`625`		`- qr/${header}redirected line pointer points to a non-heap-updated tuple at offset \d+/;`
`626`	`624`	`}`
`627`	`625`	`elsif ($offnum == 18)`
`628`	`626`	`{`
`@@ -637,10 +635,9 @@ sub header`
`637`	`635`	`}`
`638`	`636`	`elsif ($offnum == 19)`
`639`	`637`	`{`
`640`		`- # unset HEAP_ONLY_TUPLE and HEAP_UPDATED flag, so that update chain`
`641`		`- # validation will complain about offset 17`
	`638`	`+ # unset HEAP_ONLY_TUPLE flag, so that update chain validation will`
	`639`	`+ # complain about offset 17`
`642`	`640`	`$tup->{t_infomask2} &= ~HEAP_ONLY_TUPLE;`
`643`		`- $tup->{t_infomask} &= ~HEAP_UPDATED;`
`644`	`641`	`}`
`645`	`642`	`elsif ($offnum == 22)`
`646`	`643`	`{`
`@@ -688,6 +685,8 @@ sub header`
`688`	`685`	`$tup->{t_infomask2} \|= HEAP_ONLY_TUPLE;`
`689`	`686`	`push @expected,`
`690`	`687`	`qr/${header}tuple is root of chain but is marked as heap-only tuple/;`
	`688`	`+ push @expected,`
	`689`	`+ qr/${header}tuple is heap only, but not the result of an update/;`
`691`	`690`	`}`
`692`	`691`	`elsif ($offnum == 33)`
`693`	`692`	`{`