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"}}) +}