Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

release-21.1: sql: unsafe_upsert_descriptor to bump descriptor ID sequence #69956

Closed
wants to merge 1 commit into from
Closed

release-21.1: sql: unsafe_upsert_descriptor to bump descriptor ID sequence #69956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions pkg/sql/repair.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,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
139 changes: 93 additions & 46 deletions pkg/sql/tests/repair_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func TestDescriptorRepairOrphanedDescriptors(t *testing.T) {
descs.ValidateOnWriteEnabled.Override(&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
}
Expand Down Expand Up @@ -130,7 +130,7 @@ func TestDescriptorRepairOrphanedDescriptors(t *testing.T) {
descs.ValidateOnWriteEnabled.Override(&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
}
Expand Down Expand Up @@ -265,55 +265,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 @@ -331,7 +319,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 @@ -340,7 +328,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 @@ -354,7 +342,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 @@ -364,7 +352,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 @@ -379,7 +367,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 @@ -389,7 +377,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 @@ -408,7 +396,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 @@ -418,7 +406,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 @@ -549,10 +537,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 @@ -596,12 +584,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 @@ -614,7 +598,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 @@ -645,15 +629,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 @@ -667,7 +663,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 @@ -701,3 +697,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(&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"}})
}