From 64c1f6c64e2f0c8e0258913a0e75b9ee859f1bd8 Mon Sep 17 00:00:00 2001 From: Annie Pompa Date: Tue, 10 Oct 2023 11:32:23 -0400 Subject: [PATCH] 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 --- .../catalog/post_deserialization_changes.go | 5 ++ pkg/upgrade/upgrades/first_upgrade.go | 6 +- pkg/upgrade/upgrades/first_upgrade_test.go | 79 ++++++++++++++++--- .../upgrades/grant_execute_to_public.go | 3 + 4 files changed, 78 insertions(+), 15 deletions(-) diff --git a/pkg/sql/catalog/post_deserialization_changes.go b/pkg/sql/catalog/post_deserialization_changes.go index 342435539d52..777c37fcf3ae 100644 --- a/pkg/sql/catalog/post_deserialization_changes.go +++ b/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)) }) diff --git a/pkg/upgrade/upgrades/first_upgrade.go b/pkg/upgrade/upgrades/first_upgrade.go index 5bd230fdc931..1505eb817023 100644 --- a/pkg/upgrade/upgrades/first_upgrade.go +++ b/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()) } diff --git a/pkg/upgrade/upgrades/first_upgrade_test.go b/pkg/upgrade/upgrades/first_upgrade_test.go index c87ff85f2c26..e4aeee818c48 100644 --- a/pkg/upgrade/upgrades/first_upgrade_test.go +++ b/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"}}) diff --git a/pkg/upgrade/upgrades/grant_execute_to_public.go b/pkg/upgrade/upgrades/grant_execute_to_public.go index c0367199646a..c5ac1c2c6141 100644 --- a/pkg/upgrade/upgrades/grant_execute_to_public.go +++ b/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},