Close race condition between datfrozen and relfrozen updates.

nmisch · nmisch · commit 92685c389d54 · 2024-04-29T10:24:59.000-07:00
vac_update_datfrozenxid() did multiple loads of relfrozenxid and relminmxid from buffer memory, and it assumed each would get the same value. Not so if a concurrent vac_update_relstats() did an inplace update. Commit 2d2e40e fixed the same kind of bug in vac_truncate_clog(). Today's bug could cause the rel-level field and XIDs in the rel's rows to precede the db-level field. A cluster having such values should VACUUM affected tables. Back-patch to v12 (all supported versions). Discussion: https://postgr.es/m/20240423003956.e7.nmisch@google.com
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
@@ -1633,6 +1633,8 @@ vac_update_datfrozenxid(void)
 	/*
 	 * We must seqscan pg_class to find the minimum Xid, because there is no
 	 * index that can help us here.
+	 *
+	 * See vac_truncate_clog() for the race condition to prevent.
 	 */
 	relation = table_open(RelationRelationId, AccessShareLock);
 
@@ -1641,7 +1643,9 @@ vac_update_datfrozenxid(void)
 
 	while ((classTup = systable_getnext(scan)) != NULL)
 	{
-		Form_pg_class classForm = (Form_pg_class) GETSTRUCT(classTup);
+		volatile FormData_pg_class *classForm = (Form_pg_class) GETSTRUCT(classTup);
+		TransactionId relfrozenxid = classForm->relfrozenxid;
+		TransactionId relminmxid = classForm->relminmxid;
 
 		/*
 		 * Only consider relations able to hold unfrozen XIDs (anything else
@@ -1651,8 +1655,8 @@ vac_update_datfrozenxid(void)
 			classForm->relkind != RELKIND_MATVIEW &&
 			classForm->relkind != RELKIND_TOASTVALUE)
 		{
-			Assert(!TransactionIdIsValid(classForm->relfrozenxid));
-			Assert(!MultiXactIdIsValid(classForm->relminmxid));
+			Assert(!TransactionIdIsValid(relfrozenxid));
+			Assert(!MultiXactIdIsValid(relminmxid));
 			continue;
 		}
 
@@ -1671,34 +1675,34 @@ vac_update_datfrozenxid(void)
 		 * before those relations have been scanned and cleaned up.
 		 */
 
-		if (TransactionIdIsValid(classForm->relfrozenxid))
+		if (TransactionIdIsValid(relfrozenxid))
 		{
-			Assert(TransactionIdIsNormal(classForm->relfrozenxid));
+			Assert(TransactionIdIsNormal(relfrozenxid));
 
 			/* check for values in the future */
-			if (TransactionIdPrecedes(lastSaneFrozenXid, classForm->relfrozenxid))
+			if (TransactionIdPrecedes(lastSaneFrozenXid, relfrozenxid))
 			{
 				bogus = true;
 				break;
 			}
 
 			/* determine new horizon */
-			if (TransactionIdPrecedes(classForm->relfrozenxid, newFrozenXid))
-				newFrozenXid = classForm->relfrozenxid;
+			if (TransactionIdPrecedes(relfrozenxid, newFrozenXid))
+				newFrozenXid = relfrozenxid;
 		}
 
-		if (MultiXactIdIsValid(classForm->relminmxid))
+		if (MultiXactIdIsValid(relminmxid))
 		{
 			/* check for values in the future */
-			if (MultiXactIdPrecedes(lastSaneMinMulti, classForm->relminmxid))
+			if (MultiXactIdPrecedes(lastSaneMinMulti, relminmxid))
 			{
 				bogus = true;
 				break;
 			}
 
 			/* determine new horizon */
-			if (MultiXactIdPrecedes(classForm->relminmxid, newMinMulti))
-				newMinMulti = classForm->relminmxid;
+			if (MultiXactIdPrecedes(relminmxid, newMinMulti))
+				newMinMulti = relminmxid;
 		}
 	}
 

Original file line number	Diff line number	Diff line change
`@@ -1633,6 +1633,8 @@ vac_update_datfrozenxid(void)`
`1633`	`1633`	`/*`
`1634`	`1634`	`* We must seqscan pg_class to find the minimum Xid, because there is no`
`1635`	`1635`	`* index that can help us here.`
	`1636`	`+ *`
	`1637`	`+ * See vac_truncate_clog() for the race condition to prevent.`
`1636`	`1638`	`*/`
`1637`	`1639`	`relation = table_open(RelationRelationId, AccessShareLock);`
`1638`	`1640`
`@@ -1641,7 +1643,9 @@ vac_update_datfrozenxid(void)`
`1641`	`1643`
`1642`	`1644`	`while ((classTup = systable_getnext(scan)) != NULL)`
`1643`	`1645`	`{`
`1644`		`- Form_pg_class classForm = (Form_pg_class) GETSTRUCT(classTup);`
	`1646`	`+ volatile FormData_pg_class *classForm = (Form_pg_class) GETSTRUCT(classTup);`
	`1647`	`+ TransactionId relfrozenxid = classForm->relfrozenxid;`
	`1648`	`+ TransactionId relminmxid = classForm->relminmxid;`
`1645`	`1649`
`1646`	`1650`	`/*`
`1647`	`1651`	`* Only consider relations able to hold unfrozen XIDs (anything else`
`@@ -1651,8 +1655,8 @@ vac_update_datfrozenxid(void)`
`1651`	`1655`	`classForm->relkind != RELKIND_MATVIEW &&`
`1652`	`1656`	`classForm->relkind != RELKIND_TOASTVALUE)`
`1653`	`1657`	`{`
`1654`		`- Assert(!TransactionIdIsValid(classForm->relfrozenxid));`
`1655`		`- Assert(!MultiXactIdIsValid(classForm->relminmxid));`
	`1658`	`+ Assert(!TransactionIdIsValid(relfrozenxid));`
	`1659`	`+ Assert(!MultiXactIdIsValid(relminmxid));`
`1656`	`1660`	`continue;`
`1657`	`1661`	`}`
`1658`	`1662`
`@@ -1671,34 +1675,34 @@ vac_update_datfrozenxid(void)`
`1671`	`1675`	`* before those relations have been scanned and cleaned up.`
`1672`	`1676`	`*/`
`1673`	`1677`
`1674`		`- if (TransactionIdIsValid(classForm->relfrozenxid))`
	`1678`	`+ if (TransactionIdIsValid(relfrozenxid))`
`1675`	`1679`	`{`
`1676`		`- Assert(TransactionIdIsNormal(classForm->relfrozenxid));`
	`1680`	`+ Assert(TransactionIdIsNormal(relfrozenxid));`
`1677`	`1681`
`1678`	`1682`	`/* check for values in the future */`
`1679`		`- if (TransactionIdPrecedes(lastSaneFrozenXid, classForm->relfrozenxid))`
	`1683`	`+ if (TransactionIdPrecedes(lastSaneFrozenXid, relfrozenxid))`
`1680`	`1684`	`{`
`1681`	`1685`	`bogus = true;`
`1682`	`1686`	`break;`
`1683`	`1687`	`}`
`1684`	`1688`
`1685`	`1689`	`/* determine new horizon */`
`1686`		`- if (TransactionIdPrecedes(classForm->relfrozenxid, newFrozenXid))`
`1687`		`- newFrozenXid = classForm->relfrozenxid;`
	`1690`	`+ if (TransactionIdPrecedes(relfrozenxid, newFrozenXid))`
	`1691`	`+ newFrozenXid = relfrozenxid;`
`1688`	`1692`	`}`
`1689`	`1693`
`1690`		`- if (MultiXactIdIsValid(classForm->relminmxid))`
	`1694`	`+ if (MultiXactIdIsValid(relminmxid))`
`1691`	`1695`	`{`
`1692`	`1696`	`/* check for values in the future */`
`1693`		`- if (MultiXactIdPrecedes(lastSaneMinMulti, classForm->relminmxid))`
	`1697`	`+ if (MultiXactIdPrecedes(lastSaneMinMulti, relminmxid))`
`1694`	`1698`	`{`
`1695`	`1699`	`bogus = true;`
`1696`	`1700`	`break;`
`1697`	`1701`	`}`
`1698`	`1702`
`1699`	`1703`	`/* determine new horizon */`
`1700`		`- if (MultiXactIdPrecedes(classForm->relminmxid, newMinMulti))`
`1701`		`- newMinMulti = classForm->relminmxid;`
	`1704`	`+ if (MultiXactIdPrecedes(relminmxid, newMinMulti))`
	`1705`	`+ newMinMulti = relminmxid;`
`1702`	`1706`	`}`
`1703`	`1707`	`}`
`1704`	`1708`