upgrades: assert descriptor repair has correct set of targets

This patch adds a test to assert that during automated repair of
corrupt descriptors, we do not try to repair a descriptor that
is not corrupted.

Epic: none
Fixes: #110906

Release note: None

pkg/sql/catalog/post_deserialization_changes.go

@@ -31,6 +31,11 @@ func (c *PostDeserializationChanges) Add(change PostDeserializationChangeType) {
 	c.s.Add(int(change))
 }
 
+// Len returns length of the set of changes.
+func (c PostDeserializationChanges) Len() int {
+	return c.s.Len()
+}
+
 // ForEach calls f for every change in the set of changes.
 func (c PostDeserializationChanges) ForEach(f func(change PostDeserializationChangeType)) {
 	c.s.ForEach(func(i int) { f(PostDeserializationChangeType(i)) })

pkg/upgrade/upgrades/first_upgrade.go

@@ -57,7 +57,8 @@ func FirstUpgradeFromRelease(
 	var batch catalog.DescriptorIDSet
 	const batchSize = 1000
 	if err := all.ForEachDescriptor(func(desc catalog.Descriptor) error {
-		if !desc.GetPostDeserializationChanges().HasChanges() {
+		changes := desc.GetPostDeserializationChanges()
+		if !changes.HasChanges() || (changes.Len() == 1 && changes.Contains(catalog.SetModTimeToMVCCTimestamp)) {
 			return nil
 		}
 		batch.Add(desc.GetID())
@@ -89,9 +90,6 @@ func upgradeDescriptors(
 		}
 		b := txn.KV().NewBatch()
 		for _, mut := range muts {
-			if !mut.GetPostDeserializationChanges().HasChanges() {
-				continue
-			}
 			key := catalogkeys.MakeDescMetadataKey(d.Codec, mut.GetID())
 			b.CPut(key, mut.DescriptorProto(), mut.GetRawBytesInStorage())
 		}

pkg/upgrade/upgrades/first_upgrade_test.go

@@ -75,7 +75,8 @@ func TestFirstUpgrade(t *testing.T) {
 		"CREATE TABLE foo (i INT PRIMARY KEY, j INT, INDEX idx(j))",
 	)
 
-	// Corrupt the table descriptor in an unrecoverable manner.
+	// 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 {
@@ -116,8 +117,13 @@ func TestFirstUpgrade(t *testing.T) {
 		}))
 		return b.BuildImmutable()
 	}
+
+	// Confirm that the only change undergone is the modification time being set to
+	// the MVCC timestamp.
 	require.False(t, readDescFromStorage().GetModificationTime().IsEmpty())
-	require.True(t, readDescFromStorage().GetPostDeserializationChanges().HasChanges())
+	changes := readDescFromStorage().GetPostDeserializationChanges()
+	require.Equal(t, changes.Len(), 1)
+	require.True(t, changes.Contains(catalog.SetModTimeToMVCCTimestamp))
 
 	// Wait long enough for precondition check to see the unbroken table descriptor.
 	execStmts(t, "CREATE DATABASE test3")
@@ -126,9 +132,12 @@ func TestFirstUpgrade(t *testing.T) {
 	// Upgrade the cluster version.
 	tdb.Exec(t, qUpgrade)
 
-	// The table descriptor protobuf should have the modification time set.
+	// The table descriptor protobuf should still have the modification time set;
+	// the only post-deserialization change should be SetModTimeToMVCCTimestamp.
 	require.False(t, readDescFromStorage().GetModificationTime().IsEmpty())
-	require.False(t, readDescFromStorage().GetPostDeserializationChanges().HasChanges())
+	changes = readDescFromStorage().GetPostDeserializationChanges()
+	require.Equal(t, changes.Len(), 1)
+	require.True(t, changes.Contains(catalog.SetModTimeToMVCCTimestamp))
 }
 
 // TestFirstUpgradeRepair tests the correct repair behavior of upgrade
@@ -166,22 +175,35 @@ func TestFirstUpgradeRepair(t *testing.T) {
 		}
 	}
 
-	// Create a table and a function for this test.
 	execStmts(t,
 		"CREATE DATABASE test",
 		"USE test",
+		// Create a table and function that we will corrupt for this test.
 		"CREATE TABLE foo (i INT PRIMARY KEY, j INT, INDEX idx(j))",
 		"INSERT INTO foo VALUES (1, 2)",
 		"CREATE FUNCTION test.public.f() RETURNS INT LANGUAGE SQL AS $$ SELECT 1 $$",
+		// Create the following to cover more descriptor types - ensure that said descriptors do not get repaired.
+		"CREATE TABLE bar (i INT PRIMARY KEY, j INT, INDEX idx(j))",
+		"CREATE SCHEMA bar",
+		"CREATE TYPE bar.bar AS ENUM ('hello')",
+		"CREATE FUNCTION bar.bar(a INT) RETURNS INT AS 'SELECT a*a' LANGUAGE SQL",
 	)
 
-	// Corrupt FK back references in the test table descriptor.
+	dbDesc := desctestutils.TestingGetDatabaseDescriptor(kvDB, keys.SystemSQLCodec, "test")
+	tblDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "test", "bar")
+	schemaDesc := desctestutils.TestingGetSchemaDescriptor(kvDB, keys.SystemSQLCodec, dbDesc.GetID(), "bar")
+	typDesc := desctestutils.TestingGetTypeDescriptor(kvDB, keys.SystemSQLCodec, "test", "bar", "bar")
+	fnDesc := desctestutils.TestingGetFunctionDescriptor(kvDB, keys.SystemSQLCodec, "test", "bar", "bar")
+	nonCorruptDescs := []catalog.Descriptor{dbDesc, tblDesc, schemaDesc, typDesc, fnDesc}
+
+	// Corrupt FK back references in the test table descriptor, foo.
 	codec := keys.SystemSQLCodec
-	tbl := desctestutils.TestingGetPublicTableDescriptor(kvDB, codec, "test", "foo")
-	fn := desctestutils.TestingGetFunctionDescriptor(kvDB, codec, "test", "public", "f")
+	fooTbl := desctestutils.TestingGetPublicTableDescriptor(kvDB, codec, "test", "foo")
+	fooFn := desctestutils.TestingGetFunctionDescriptor(kvDB, codec, "test", "public", "f")
+	corruptDescs := []catalog.Descriptor{fooTbl, fooFn}
 	require.NoError(t, kvDB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
 		b := txn.NewBatch()
-		tbl := tabledesc.NewBuilder(tbl.TableDesc()).BuildExistingMutableTable()
+		tbl := tabledesc.NewBuilder(fooTbl.TableDesc()).BuildExistingMutableTable()
 		tbl.InboundFKs = []descpb.ForeignKeyConstraint{{
 			OriginTableID:       123456789,
 			OriginColumnIDs:     tbl.PublicColumnIDs(), // Used such that len(OriginColumnIDs) == len(PublicColumnIDs)
@@ -193,7 +215,7 @@ func TestFirstUpgradeRepair(t *testing.T) {
 		}}
 		tbl.NextConstraintID++
 		b.Put(catalogkeys.MakeDescMetadataKey(codec, tbl.GetID()), tbl.DescriptorProto())
-		fn := funcdesc.NewBuilder(fn.FuncDesc()).BuildExistingMutableFunction()
+		fn := funcdesc.NewBuilder(fooFn.FuncDesc()).BuildExistingMutableFunction()
 		fn.DependedOnBy = []descpb.FunctionDescriptor_Reference{{
 			ID:        123456789,
 			ColumnIDs: []descpb.ColumnID{1},
@@ -202,6 +224,27 @@ func TestFirstUpgradeRepair(t *testing.T) {
 		return txn.Run(ctx, b)
 	}))
 
+	readDescFromStorage := func(descID descpb.ID) catalog.Descriptor {
+		descKey := catalogkeys.MakeDescMetadataKey(keys.SystemSQLCodec, descID)
+		var b catalog.DescriptorBuilder
+		require.NoError(t, kvDB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
+			v, err := txn.Get(ctx, descKey)
+			if err != nil {
+				return err
+			}
+			b, err = descbuilder.FromSerializedValue(v.Value)
+			return err
+		}))
+		return b.BuildImmutable()
+	}
+
+	descOldVersionMap := make(map[descpb.ID]descpb.DescriptorVersion)
+
+	for _, desc := range append(nonCorruptDescs, corruptDescs...) {
+		descId := desc.GetID()
+		descOldVersionMap[descId] = readDescFromStorage(descId).GetVersion()
+	}
+
 	// The corruption should remain undetected for DML queries.
 	tdb.CheckQueryResults(t, "SELECT * FROM test.public.foo", [][]string{{"1", "2"}})
 	tdb.CheckQueryResults(t, "SELECT test.public.f()", [][]string{{"1"}})
@@ -232,7 +275,21 @@ func TestFirstUpgradeRepair(t *testing.T) {
 	tdb.CheckQueryResults(t, qDetectCorruption, [][]string{{"0"}})
 	tdb.CheckQueryResults(t, qDetectRepairableCorruption, [][]string{{"0"}})
 
-	// Check that the table and function are OK.
+	// 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())
+	}
+
+	// Assert that no version upgrade is reflected for non-repaired descriptors.
+	for _, d := range nonCorruptDescs {
+		descId := d.GetID()
+		desc := readDescFromStorage(descId)
+		require.Equalf(t, descOldVersionMap[descId], desc.GetVersion(), desc.GetName())
+	}
+
+	// Check that the repaired table and function are OK.
 	tdb.CheckQueryResults(t, "SELECT * FROM test.public.foo", [][]string{{"1", "2"}})
 	tdb.Exec(t, "ALTER TABLE test.foo ADD COLUMN k INT DEFAULT 42")
 	tdb.CheckQueryResults(t, "SELECT * FROM test.public.foo", [][]string{{"1", "2", "42"}})

pkg/upgrade/upgrades/grant_execute_to_public.go

@@ -80,6 +80,9 @@ func grantExecuteToPublicOnAllFunctions(
 			}
 			kvBatch := txn.KV().NewBatch()
 			for _, desc := range descs {
+				if desc.GetPrivileges().CheckPrivilege(username.PublicRoleName(), privilege.EXECUTE) {
+					continue
+				}
 				desc.GetPrivileges().Grant(
 					username.PublicRoleName(),
 					privilege.List{privilege.EXECUTE},