Skip to content

Commit

Permalink
Merge cockroachdb#68662
Browse files Browse the repository at this point in the history 
68662: sql: unsafe_upsert_descriptor to bump descriptor ID sequence r=postamar a=postamar

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 cockroachdb#68230.

Release note: None

Co-authored-by: Marius Posta <[email protected]>
  • Loading branch information
craig[bot] and Marius Posta committed Aug 13, 2021
2 parents 7018384 + 0323719 commit 8adbb79
Show file tree
Hide file tree
Showing 2 changed files with 117 additions and 52 deletions.
22 changes: 22 additions & 0 deletions pkg/sql/repair.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down
147 changes: 95 additions & 52 deletions pkg/sql/tests/repair_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,21 +73,19 @@ 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
}
_, err := tx.Exec("SELECT crdb_internal.unsafe_upsert_namespace_entry($1, $2, $3, $4, true);",
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
Expand Down Expand Up @@ -124,21 +122,19 @@ 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
}
_, err := tx.Exec("SELECT crdb_internal.unsafe_upsert_namespace_entry($1, $2, $3, $4, true);",
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
Expand Down Expand Up @@ -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"\]`,
},
},
},
Expand 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"`,
Expand All @@ -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{
Expand All @@ -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);`,
Expand All @@ -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);`,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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);`,
Expand All @@ -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);`,
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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": [
Expand All @@ -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,
Expand Down Expand Up @@ -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 } } ],
Expand All @@ -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,
Expand Down Expand Up @@ -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"}})
}

0 comments on commit 8adbb79

Please sign in to comment.