Don't test HEAP_XMAX_INVALID when freezing xmax.

petergeoghegan · petergeoghegan · commit 02d647bbf057 · 2022-11-23T10:49:39.000-08:00
We shouldn't ever need to rely on whether HEAP_XMAX_INVALID is set in t_infomask when considering whether or not an xmax should be deemed already frozen, since that status flag is just a hint. The only acceptable representation for an "xmax_already_frozen" raw xmax field is the transaction ID value zero (also known as InvalidTransactionId). Adjust code that superficially appeared to rely on HEAP_XMAX_INVALID to make the rule about xmax_already_frozen clear. Also avoid needlessly rereading the tuple's raw xmax. Oversight in bugfix commit d2599ec. There is no evidence that this ever led to incorrect behavior, so no backpatch. The worst consequence of this bug was that VACUUM could hypothetically fail to notice and report on certain kinds of corruption, which seems fairly benign. Author: Peter Geoghegan <pg@bowt.ie> Discussion: https://postgr.es/m/CAH2-Wzkh3DMCDRPfhZxj9xCq9v3WmzvmbiCpf1dNKUBPadhCbQ@mail.gmail.com
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
@@ -6542,6 +6542,7 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
 
 	if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
 	{
+		/* Raw xmax is a MultiXactId */
 		TransactionId newxmax;
 		uint16		flags;
 		TransactionId mxid_oldest_xid_out = *relfrozenxid_out;
@@ -6640,6 +6641,7 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
 	}
 	else if (TransactionIdIsNormal(xid))
 	{
+		/* Raw xmax is normal XID */
 		if (TransactionIdPrecedes(xid, relfrozenxid))
 			ereport(ERROR,
 					(errcode(ERRCODE_DATA_CORRUPTED),
@@ -6670,9 +6672,10 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
 				*relfrozenxid_out = xid;
 		}
 	}
-	else if ((tuple->t_infomask & HEAP_XMAX_INVALID) ||
-			 !TransactionIdIsValid(HeapTupleHeaderGetRawXmax(tuple)))
+	else if (!TransactionIdIsValid(xid))
 	{
+		/* Raw xmax is InvalidTransactionId XID */
+		Assert((tuple->t_infomask & HEAP_XMAX_IS_MULTI) == 0);
 		freeze_xmax = false;
 		xmax_already_frozen = true;
 		/* No need for relfrozenxid_out handling for already-frozen xmax */

Original file line number	Diff line number	Diff line change
`@@ -6542,6 +6542,7 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,`
`6542`	`6542`
`6543`	`6543`	`if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)`
`6544`	`6544`	`{`
	`6545`	`+ /* Raw xmax is a MultiXactId */`
`6545`	`6546`	`TransactionId newxmax;`
`6546`	`6547`	`uint16 flags;`
`6547`	`6548`	`TransactionId mxid_oldest_xid_out = *relfrozenxid_out;`
`@@ -6640,6 +6641,7 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,`
`6640`	`6641`	`}`
`6641`	`6642`	`else if (TransactionIdIsNormal(xid))`
`6642`	`6643`	`{`
	`6644`	`+ /* Raw xmax is normal XID */`
`6643`	`6645`	`if (TransactionIdPrecedes(xid, relfrozenxid))`
`6644`	`6646`	`ereport(ERROR,`
`6645`	`6647`	`(errcode(ERRCODE_DATA_CORRUPTED),`
`@@ -6670,9 +6672,10 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,`
`6670`	`6672`	`*relfrozenxid_out = xid;`
`6671`	`6673`	`}`
`6672`	`6674`	`}`
`6673`		`- else if ((tuple->t_infomask & HEAP_XMAX_INVALID) \|\|`
`6674`		`- !TransactionIdIsValid(HeapTupleHeaderGetRawXmax(tuple)))`
	`6675`	`+ else if (!TransactionIdIsValid(xid))`
`6675`	`6676`	`{`
	`6677`	`+ /* Raw xmax is InvalidTransactionId XID */`
	`6678`	`+ Assert((tuple->t_infomask & HEAP_XMAX_IS_MULTI) == 0);`
`6676`	`6679`	`freeze_xmax = false;`
`6677`	`6680`	`xmax_already_frozen = true;`
`6678`	`6681`	`/* No need for relfrozenxid_out handling for already-frozen xmax */`