From 0323719b65e0de74d8d46cec44af8f5164b10845 Mon Sep 17 00:00:00 2001 From: Marius Posta Date: Tue, 10 Aug 2021 14:28:26 -0400 Subject: [PATCH] sql: unsafe_upsert_descriptor to bump descriptor ID sequence Previously, it was possible to use this descriptor repair function to insert a new descriptor with an ID greater than any descriptor in the cluster and would not update the descriptor ID sequence value. Subsequent CREATE statements would therefore write descriptors with IDs smaller or equal to the descriptor inserted previously. In the equal case, this corrupts the schema. This commit adds a test that checks that the ID is less than the value stored in the descriptor ID sequence. When that is not the case: - an error is returned when the `force` flag is not set, - otherwise, the sequence value is incremented appropriately. Fixes #68230. Release note: None --- pkg/sql/repair.go | 22 ++++++ pkg/sql/tests/repair_test.go | 147 ++++++++++++++++++++++------------- 2 files changed, 117 insertions(+), 52 deletions(-) diff --git a/pkg/sql/repair.go b/pkg/sql/repair.go index 8f31e735f088..1d8cf28cd016 100644 --- a/pkg/sql/repair.go +++ b/pkg/sql/repair.go @@ -161,6 +161,28 @@ func (p *planner) UnsafeUpsertDescriptor( objectType = privilege.Schema } + // Check that the descriptor ID is less than the counter used for creating new + // descriptor IDs. If not, and if the force flag is set, increment it. + maxDescIDKeyVal, err := p.extendedEvalCtx.DB.Get(context.Background(), p.extendedEvalCtx.Codec.DescIDSequenceKey()) + if err != nil { + return err + } + maxDescID, err := maxDescIDKeyVal.Value.GetInt() + if err != nil { + return err + } + if maxDescID <= descID { + if !force { + return pgerror.Newf(pgcode.InvalidObjectDefinition, + "descriptor ID %d must be less than the descriptor ID sequence value %d", descID, maxDescID) + } + inc := descID - maxDescID + 1 + _, err = kv.IncrementValRetryable(ctx, p.extendedEvalCtx.DB, p.extendedEvalCtx.Codec.DescIDSequenceKey(), inc) + if err != nil { + return err + } + } + if force { p.Descriptors().SkipValidationOnWrite() } diff --git a/pkg/sql/tests/repair_test.go b/pkg/sql/tests/repair_test.go index 9d15a586e0cb..5684eea7cb74 100644 --- a/pkg/sql/tests/repair_test.go +++ b/pkg/sql/tests/repair_test.go @@ -73,13 +73,12 @@ func TestDescriptorRepairOrphanedDescriptors(t *testing.T) { // descriptor and namespace entry. This would normally be unsafe because // it would leave table data around. t.Run("orphaned view - 51782", func(t *testing.T) { - s, db, cleanup := setup(t) + _, db, cleanup := setup(t) defer cleanup() - descs.ValidateOnWriteEnabled.Override(ctx, &s.ClusterSettings().SV, false) require.NoError(t, crdb.ExecuteTx(ctx, db, nil, func(tx *gosql.Tx) error { if _, err := tx.Exec( - "SELECT crdb_internal.unsafe_upsert_descriptor($1, decode($2, 'hex'));", + "SELECT crdb_internal.unsafe_upsert_descriptor($1, decode($2, 'hex'), true);", descID, orphanedTable); err != nil { return err } @@ -87,7 +86,6 @@ func TestDescriptorRepairOrphanedDescriptors(t *testing.T) { parentID, schemaID, tableName, descID) return err })) - descs.ValidateOnWriteEnabled.Override(ctx, &s.ClusterSettings().SV, true) // Ideally we should be able to query `crdb_internal.invalid_object` but it // does not do enough validation. Instead we'll just observe the issue that @@ -124,13 +122,12 @@ func TestDescriptorRepairOrphanedDescriptors(t *testing.T) { // will then repair it by injecting a new database descriptor and namespace // entry and then demonstrate the problem is resolved. t.Run("orphaned table with data - 51782", func(t *testing.T) { - s, db, cleanup := setup(t) + _, db, cleanup := setup(t) defer cleanup() - descs.ValidateOnWriteEnabled.Override(ctx, &s.ClusterSettings().SV, false) require.NoError(t, crdb.ExecuteTx(ctx, db, nil, func(tx *gosql.Tx) error { if _, err := tx.Exec( - "SELECT crdb_internal.unsafe_upsert_descriptor($1, decode($2, 'hex'));", + "SELECT crdb_internal.unsafe_upsert_descriptor($1, decode($2, 'hex'), true);", descID, orphanedTable); err != nil { return err } @@ -138,7 +135,6 @@ func TestDescriptorRepairOrphanedDescriptors(t *testing.T) { parentID, schemaID, tableName, descID) return err })) - descs.ValidateOnWriteEnabled.Override(ctx, &s.ClusterSettings().SV, true) // Ideally we should be able to query `crdb_internal.invalid_objects` but it // does not do enough validation. Instead we'll just observe the issue that @@ -266,55 +262,43 @@ func TestDescriptorRepair(t *testing.T) { { // 1 before: []string{ `CREATE DATABASE test`, - `SELECT crdb_internal.unsafe_upsert_namespace_entry(52, 29, 'foo', 59, true)`, + `CREATE TABLE test.foo ()`, }, - op: upsertRepair, + op: upsertRepairNoForce, expEventLogEntries: []eventLogPattern{ { typ: "unsafe_upsert_descriptor", - info: `"DescriptorID":59`, - }, - { - typ: "alter_table_owner", - info: `"DescriptorID":59,"TableName":"foo","Owner":"root"`, + info: `"DescriptorID":53`, }, { typ: "change_table_privilege", - info: `"DescriptorID":59,"Grantee":"root","GrantedPrivileges":\["ALL"\]`, + info: `"DescriptorID":53,"Grantee":"newuser1","GrantedPrivileges":\["ALL"\]`, }, { typ: "change_table_privilege", - info: `"DescriptorID":59,"Grantee":"admin","GrantedPrivileges":\["ALL"\]`, - }, - { - typ: "change_table_privilege", - info: `"DescriptorID":59,"Grantee":"newuser1","GrantedPrivileges":\["ALL"\]`, - }, - { - typ: "change_table_privilege", - info: `"DescriptorID":59,"Grantee":"newuser2","GrantedPrivileges":\["ALL"\]`, + info: `"DescriptorID":53,"Grantee":"newuser2","GrantedPrivileges":\["ALL"\]`, }, }, }, { // 2 before: []string{ `CREATE DATABASE test`, - `SELECT crdb_internal.unsafe_upsert_namespace_entry(52, 29, 'foo', 59, true)`, - upsertRepair, + `SELECT crdb_internal.unsafe_upsert_namespace_entry(52, 29, 'foo', 53, true)`, + upsertRepairForce, }, op: upsertUpdatePrivileges, expEventLogEntries: []eventLogPattern{ { typ: "alter_table_owner", - info: `"DescriptorID":59,"TableName":"foo","Owner":"admin"`, + info: `"DescriptorID":53,"TableName":"foo","Owner":"admin"`, }, { typ: "change_table_privilege", - info: `"DescriptorID":59,"Grantee":"newuser1","GrantedPrivileges":\["DROP"\],"RevokedPrivileges":\["ALL"\]`, + info: `"DescriptorID":53,"Grantee":"newuser1","GrantedPrivileges":\["DROP"\],"RevokedPrivileges":\["ALL"\]`, }, { typ: "change_table_privilege", - info: `"DescriptorID":59,"Grantee":"newuser2","RevokedPrivileges":\["ALL"\]`, + info: `"DescriptorID":53,"Grantee":"newuser2","RevokedPrivileges":\["ALL"\]`, }, }, }, @@ -332,7 +316,7 @@ SELECT crdb_internal.unsafe_delete_namespace_entry("parentID", 0, 'foo', id) // Upsert a descriptor which is invalid, then try to upsert a namespace // entry for it and show that it fails. before: []string{ - upsertInvalidateDuplicateColumnDescriptor, + upsertInvalidateDuplicateColumnDescriptorBefore, }, op: `SELECT crdb_internal.unsafe_upsert_namespace_entry(50, 29, 'foo', 52);`, expErrRE: `relation "foo" \(52\): duplicate column name: "i"`, @@ -341,7 +325,7 @@ SELECT crdb_internal.unsafe_delete_namespace_entry("parentID", 0, 'foo', id) // Upsert a descriptor which is invalid, then try to upsert a namespace // entry for it and show that it succeeds with the force flag. before: []string{ - upsertInvalidateDuplicateColumnDescriptor, + upsertInvalidateDuplicateColumnDescriptorBefore, }, op: `SELECT crdb_internal.unsafe_upsert_namespace_entry(50, 29, 'foo', 52, true);`, expEventLogEntries: []eventLogPattern{ @@ -355,7 +339,7 @@ SELECT crdb_internal.unsafe_delete_namespace_entry("parentID", 0, 'foo', id) // Upsert a descriptor which is invalid, upsert a namespace entry for it, // then show that deleting the descriptor fails without the force flag. before: []string{ - upsertInvalidateDuplicateColumnDescriptor, + upsertInvalidateDuplicateColumnDescriptorBefore, `SELECT crdb_internal.unsafe_upsert_namespace_entry(50, 29, 'foo', 52, true);`, }, op: `SELECT crdb_internal.unsafe_delete_descriptor(52);`, @@ -365,7 +349,7 @@ SELECT crdb_internal.unsafe_delete_namespace_entry("parentID", 0, 'foo', id) // Upsert a descriptor which is invalid, upsert a namespace entry for it, // then show that deleting the descriptor succeeds with the force flag. before: []string{ - upsertInvalidateDuplicateColumnDescriptor, + upsertInvalidateDuplicateColumnDescriptorBefore, `SELECT crdb_internal.unsafe_upsert_namespace_entry(50, 29, 'foo', 52, true);`, }, op: `SELECT crdb_internal.unsafe_delete_descriptor(52, true);`, @@ -380,7 +364,7 @@ SELECT crdb_internal.unsafe_delete_namespace_entry("parentID", 0, 'foo', id) // Upsert a descriptor which is invalid, upsert a namespace entry for it, // then show that updating the descriptor fails without the force flag. before: []string{ - upsertInvalidateDuplicateColumnDescriptor, + upsertInvalidateDuplicateColumnDescriptorBefore, `SELECT crdb_internal.unsafe_upsert_namespace_entry(50, 29, 'foo', 52, true);`, }, op: updateInvalidateDuplicateColumnDescriptorNoForce, @@ -390,7 +374,7 @@ SELECT crdb_internal.unsafe_delete_namespace_entry("parentID", 0, 'foo', id) // Upsert a descriptor which is invalid, upsert a namespace entry for it, // then show that updating the descriptor succeeds the force flag. before: []string{ - upsertInvalidateDuplicateColumnDescriptor, + upsertInvalidateDuplicateColumnDescriptorBefore, `SELECT crdb_internal.unsafe_upsert_namespace_entry(50, 29, 'foo', 52, true);`, }, op: updateInvalidateDuplicateColumnDescriptorForce, @@ -409,7 +393,7 @@ SELECT crdb_internal.unsafe_delete_namespace_entry("parentID", 0, 'foo', id) // Upsert a descriptor which is invalid, upsert a namespace entry for it, // then show that deleting the namespace entry fails without the force flag. before: []string{ - upsertInvalidateDuplicateColumnDescriptor, + upsertInvalidateDuplicateColumnDescriptorBefore, `SELECT crdb_internal.unsafe_upsert_namespace_entry(50, 29, 'foo', 52, true);`, }, op: `SELECT crdb_internal.unsafe_delete_namespace_entry(50, 29, 'foo', 52);`, @@ -419,7 +403,7 @@ SELECT crdb_internal.unsafe_delete_namespace_entry("parentID", 0, 'foo', id) // Upsert a descriptor which is invalid, upsert a namespace entry for it, // then show that deleting the namespace entry succeeds with the force flag. before: []string{ - upsertInvalidateDuplicateColumnDescriptor, + upsertInvalidateDuplicateColumnDescriptorBefore, `SELECT crdb_internal.unsafe_upsert_namespace_entry(50, 29, 'foo', 52, true);`, }, op: `SELECT crdb_internal.unsafe_delete_namespace_entry(50, 29, 'foo', 52, true);`, @@ -550,10 +534,10 @@ const ( // This is a statement to insert the invalid descriptor above using // crdb_internal.unsafe_upsert_descriptor. - upsertInvalidateDuplicateColumnDescriptor = ` + upsertInvalidateDuplicateColumnDescriptorBefore = ` SELECT crdb_internal.unsafe_upsert_descriptor(52, crdb_internal.json_to_pb('cockroach.sql.sqlbase.Descriptor', ` + - invalidDuplicateColumnDescriptor + `))` + invalidDuplicateColumnDescriptor + `), true)` // These are CTEs for the below statements to update the above descriptor // and fix its validation problems. @@ -597,12 +581,8 @@ SELECT crdb_internal.unsafe_upsert_descriptor(52, descriptor) SELECT crdb_internal.unsafe_upsert_descriptor(52, descriptor, true) FROM updated; ` - - // This is a statement to repair an invalid descriptor using - // crdb_internal.unsafe_upsert_descriptor. - upsertRepair = ` -SELECT crdb_internal.unsafe_upsert_descriptor(59, crdb_internal.json_to_pb('cockroach.sql.sqlbase.Descriptor', -'{ + // This is the descriptor definition used in the upsertRepair* statements. + repairedDescriptor = `'{ "table": { "columns": [ { "id": 1, "name": "i", "type": { "family": "IntFamily", "oid": 20, "width": 64 } } ], "families": [ @@ -615,7 +595,7 @@ SELECT crdb_internal.unsafe_upsert_descriptor(59, crdb_internal.json_to_pb('cock } ], "formatVersion": 3, - "id": 59, + "id": 53, "name": "foo", "nextColumnId": 2, "nextFamilyId": 1, @@ -647,15 +627,27 @@ SELECT crdb_internal.unsafe_upsert_descriptor(59, crdb_internal.json_to_pb('cock "unexposedParentSchemaId": 29, "version": 1 } -} -')) -` +}'` + + // This is a statement to repair an invalid descriptor using + // crdb_internal.unsafe_upsert_descriptor. + upsertRepairNoForce = ` +SELECT crdb_internal.unsafe_upsert_descriptor(53, + crdb_internal.json_to_pb('cockroach.sql.sqlbase.Descriptor', ` + + repairedDescriptor + `), false)` + + // This is a statement to force-repair an invalid descriptor using + // crdb_internal.unsafe_upsert_descriptor. + upsertRepairForce = ` +SELECT crdb_internal.unsafe_upsert_descriptor(53, + crdb_internal.json_to_pb('cockroach.sql.sqlbase.Descriptor', ` + + repairedDescriptor + `), true)` // This is a statement to update the above descriptor's privileges. // It will change the table owner, add privileges for a new user, // alter the privilege of an existing user, and revoke all privileges for an old user. upsertUpdatePrivileges = ` -SELECT crdb_internal.unsafe_upsert_descriptor(59, crdb_internal.json_to_pb('cockroach.sql.sqlbase.Descriptor', +SELECT crdb_internal.unsafe_upsert_descriptor(53, crdb_internal.json_to_pb('cockroach.sql.sqlbase.Descriptor', '{ "table": { "columns": [ { "id": 1, "name": "i", "type": { "family": "IntFamily", "oid": 20, "width": 64 } } ], @@ -669,7 +661,7 @@ SELECT crdb_internal.unsafe_upsert_descriptor(59, crdb_internal.json_to_pb('cock } ], "formatVersion": 3, - "id": 59, + "id": 53, "name": "foo", "nextColumnId": 2, "nextFamilyId": 1, @@ -704,3 +696,54 @@ SELECT crdb_internal.unsafe_upsert_descriptor(59, crdb_internal.json_to_pb('cock ')) ` ) + +// TestDescriptorRepairIdGeneration verifies that `unsafe_upsert_descriptor` +// properly takes the descriptor ID sequence into account. Otherwise a CREATE +// statement might eventually overwrite a descriptor inserted in this way. +func TestDescriptorRepairIdGeneration(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + ctx := context.Background() + s, db, _ := serverutils.StartServer(t, base.TestServerArgs{}) + defer s.Stopper().Stop(ctx) + tdb := sqlutils.MakeSQLRunner(db) + + const q = `SELECT crdb_internal.unsafe_upsert_descriptor(123, crdb_internal.json_to_pb('cockroach.sql.sqlbase.Descriptor', $2::JSONB), $1)` + const d = `{ + "database": { + "id": 123, + "name": "foo", + "privileges": { + "ownerProto": "root", + "users": [ + { + "privileges": 2, + "userProto": "admin" + }, + { + "privileges": 2, + "userProto": "root" + } + ], + "version": 2 + }, + "version": "1" + } +}` + + // Required so test doesn't fail due to namespace validation failures. + descs.ValidateOnWriteEnabled.Override(ctx, &s.ClusterSettings().SV, false) + + // Inserting a descriptor with an ID too high should fail. + tdb.ExpectErr(t, "descriptor ID 123 must be less than the descriptor ID sequence value", q, false /* force */, d) + tdb.CheckQueryResults(t, "SELECT count(*) FROM system.descriptor WHERE id >= 123", [][]string{{"0"}}) + + // Force the insertion. + tdb.Exec(t, q, true /* force */, d) + tdb.CheckQueryResults(t, "SELECT count(*) FROM system.descriptor WHERE id >= 123", [][]string{{"1"}}) + + // Subsequent new descriptors should have an even greater ID. + tdb.Exec(t, "CREATE DATABASE bar;") + tdb.CheckQueryResults(t, "SELECT count(*) FROM system.descriptor WHERE id >= 123", [][]string{{"2"}}) +}