update: bump descriptor versions during repair

Previously, the post deserialization repair incorrectly wrote the
descriptors at the exact same version. While the leasing manager will
tolerate this for non-historical descriptors, any historical queries can
end up failing after the first upgrade. This patch adds a version bump
when running post deserialization changes to avoid these issues.

Fixes: #153358

Release note: None

Author: fqazi

pkg/upgrade/upgrades/first_upgrade.go

@@ -127,7 +127,11 @@ func upgradeDescriptors(
 				}
 				b := txn.KV().NewBatch()
 				for _, mut := range muts {
-					if !mut.GetPostDeserializationChanges().HasChanges() {
+					// Both newly created and altered descriptors never write the modification time
+					// to storage. This post-deserialization change is always expected now.
+					changes := mut.GetPostDeserializationChanges()
+					hasChanges := changes.Len() > 1 || !changes.Contains(catalog.SetModTimeToMVCCTimestamp)
+					if !hasChanges {
 						// In the upgrade to 25.4, we do a one-time rewrite of all
 						// descriptors in order to upgrade them to use the new type
 						// serialization format.
@@ -137,6 +141,7 @@ func upgradeDescriptors(
 						}
 					}
 					key := catalogkeys.MakeDescMetadataKey(d.Codec, mut.GetID())
+					mut.MaybeIncrementVersion()
 					b.CPut(key, mut.DescriptorProto(), mut.GetRawBytesInStorage())
 				}
 				return txn.KV().Run(ctx, b)

pkg/upgrade/upgrades/first_upgrade_test.go

@@ -79,14 +79,16 @@ func TestFirstUpgrade(t *testing.T) {
 		"USE test",
 		"CREATE TABLE foo (i INT PRIMARY KEY, j INT, INDEX idx(j))",
 	)
-
+	// Select a timestamp for when the tables were created.
+	startTime := testServer.Clock().Now()
 	// Corrupt the table descriptor in an unrecoverable manner. We are not able to automatically repair this
 	// descriptor.
 	tbl := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "test", "foo")
 	descKey := catalogkeys.MakeDescMetadataKey(keys.SystemSQLCodec, tbl.GetID())
 	require.NoError(t, kvDB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
 		mut := tabledesc.NewBuilder(tbl.TableDesc()).BuildExistingMutableTable()
 		mut.NextIndexID = 1
+		mut.MaybeIncrementVersion()
 		return txn.Put(ctx, descKey, mut.DescriptorProto())
 	}))
 
@@ -106,6 +108,8 @@ func TestFirstUpgrade(t *testing.T) {
 	require.NoError(t, kvDB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
 		mut := tabledesc.NewBuilder(tbl.TableDesc()).BuildExistingMutableTable()
 		mut.ModificationTime = hlc.Timestamp{}
+		mut.Indexes[0].NotVisible = true
+		mut.Version += 2 // Increment by 2 because we wrote a version above.
 		return txn.Put(ctx, descKey, mut.DescriptorProto())
 	}))
 
@@ -127,8 +131,9 @@ func TestFirstUpgrade(t *testing.T) {
 	// the MVCC timestamp.
 	require.False(t, readDescFromStorage().GetModificationTime().IsEmpty())
 	changes := readDescFromStorage().GetPostDeserializationChanges()
-	require.Equal(t, changes.Len(), 1)
+	require.Equal(t, 2, changes.Len())
 	require.True(t, changes.Contains(catalog.SetModTimeToMVCCTimestamp))
+	require.True(t, changes.Contains(catalog.SetIndexInvisibility))
 
 	// Wait long enough for precondition check to see the unbroken table descriptor.
 	execStmts(t, "CREATE DATABASE test3")
@@ -137,18 +142,18 @@ func TestFirstUpgrade(t *testing.T) {
 	// Upgrade the cluster version.
 	tdb.Exec(t, qUpgrade)
 
-	// The table descriptor protobuf should still have the modification time set;
-	// the only post-deserialization change should be SetModTimeToMVCCTimestamp.
+	// Run a schema change on the bar table.
+	tdb.Exec(t, "ALTER TABLE foo ADD COLUMN z INT")
+
+	// The only post-deserialization change should be SetModTimeToMVCCTimestamp,
+	// because the MVCC timestamp gets on each write.
 	require.False(t, readDescFromStorage().GetModificationTime().IsEmpty())
 	changes = readDescFromStorage().GetPostDeserializationChanges()
-	if v1.Equal(clusterversion.V25_4.Version()) {
-		// In 25.4, we do a one-time rewrite of all descriptors, so there should be
-		// no changes here. In later versions, there should be one change.
-		require.Equal(t, 0, changes.Len())
-	} else {
-		require.Equal(t, 1, changes.Len())
-		require.True(t, changes.Contains(catalog.SetModTimeToMVCCTimestamp))
-	}
+	require.Equal(t, 1, changes.Len())
+	require.True(t, changes.Contains(catalog.SetModTimeToMVCCTimestamp))
+	// Validate we can read bar before this upgrade. The lease manager should see
+	// distinct version numbers in the history. Otherwise, it can panic.
+	tdb.Exec(t, fmt.Sprintf("SELECT * FROM foo AS OF SYSTEM TIME '%s'", startTime.AsOfSystemTime()))
 }
 
 // TestFirstUpgradeRepair tests the correct repair behavior of upgrade
@@ -241,12 +246,14 @@ func TestFirstUpgradeRepair(t *testing.T) {
 			ConstraintID:        tbl.NextConstraintID,
 		}}
 		tbl.NextConstraintID++
+		tbl.MaybeIncrementVersion()
 		b.Put(catalogkeys.MakeDescMetadataKey(codec, tbl.GetID()), tbl.DescriptorProto())
 		fn := funcdesc.NewBuilder(fooFn.FuncDesc()).BuildExistingMutableFunction()
 		fn.DependedOnBy = []descpb.FunctionDescriptor_Reference{{
 			ID:        123456789,
 			ColumnIDs: []descpb.ColumnID{1},
 		}}
+		fn.MaybeIncrementVersion()
 		b.Put(catalogkeys.MakeDescMetadataKey(codec, fn.GetID()), fn.DescriptorProto())
 		return txn.Run(ctx, b)
 	}))
@@ -340,18 +347,34 @@ func TestFirstUpgradeRepair(t *testing.T) {
 	tdb.CheckQueryResults(t, qDetectRepairableCorruption, [][]string{{"0"}})
 	close(upgradeCompleted)
 	require.NoError(t, grp.Wait())
+	expectedVersionBump := 1
+	// Upgrades from before 25.4 will round-trip all descriptors once, so we
+	// expect two version bumps. Once for the repair and the next for the
+	// round trip.
+	if v0.Less(clusterversion.V25_4.Version()) {
+		expectedVersionBump += 1
+	}
 	// Assert that a version upgrade is reflected for repaired descriptors (stricly one version upgrade).
 	for _, d := range corruptDescs {
 		descId := d.GetID()
 		desc := readDescFromStorage(descId)
-		require.Equalf(t, descOldVersionMap[descId]+1, desc.GetVersion(), desc.GetName())
+		require.Equalf(t, descOldVersionMap[descId]+descpb.DescriptorVersion(expectedVersionBump), desc.GetVersion(), desc.GetName())
 	}
 
 	// Assert that no version upgrade is reflected for non-repaired descriptors.
+	expectedVersionBump = 0
+	// Upgrading from before 25.4 all non-database descriptors will be round tripped.
+	if v0.Less(clusterversion.V25_4.Version()) {
+		expectedVersionBump += 1
+	}
 	for _, d := range nonCorruptDescs {
 		descId := d.GetID()
 		desc := readDescFromStorage(descId)
-		require.Equalf(t, descOldVersionMap[descId], desc.GetVersion(), desc.GetName())
+		if desc.DescriptorType() != catalog.Database {
+			require.Equalf(t, descOldVersionMap[descId]+descpb.DescriptorVersion(expectedVersionBump), desc.GetVersion(), desc.GetName())
+		} else {
+			require.Equalf(t, descOldVersionMap[descId], desc.GetVersion(), desc.GetName())
+		}
 	}
 
 	// Check that the repaired table and function are OK.