Skip to content

Commit

Permalink
Merge cockroachdb#92076 cockroachdb#92081
Browse files Browse the repository at this point in the history 
92076: sql: DROP TENANT makes name reclaimable r=stevendanna a=stevendanna

Previously, you couldn't reuse a tenant name until GC completed. This makes the name immediately reclaimable, saving the old name in droppedName in case we need it for something.

Note that with the virtual column this only works because ->> returns NULL for the empty string.

Epic: None

Release note: None

92081: backup-restore: fix flake in datadriven test r=msbutler a=adityamaru

Fixes: cockroachdb#92077

Release note: None

Co-authored-by: Steven Danna <[email protected]>
Co-authored-by: adityamaru <[email protected]>
  • Loading branch information
@stevendanna @adityamaru
3 people committed Nov 17, 2022
3 parents 5179a84 + cf0b2de + ccd874d commit 98197fd
Show file tree
Hide file tree
Showing 10 changed files with 95 additions and 63 deletions.
36 changes: 18 additions & 18 deletions pkg/ccl/backupccl/backup_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6822,14 +6822,14 @@ func TestBackupRestoreTenant(t *testing.T) {
restoreDB := sqlutils.MakeSQLRunner(restoreTC.Conns[0])

restoreDB.CheckQueryResults(t, `select id, active, name, crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantInfo', info, true) from system.tenants`, [][]string{
{`1`, `true`, `system`, `{"id": "1", "name": "system", "state": "ACTIVE"}`},
{`1`, `true`, `system`, `{"droppedName": "", "id": "1", "name": "system", "state": "ACTIVE"}`},
})
restoreDB.Exec(t, `RESTORE TENANT 10 FROM 'nodelocal://1/t10'`)
restoreDB.CheckQueryResults(t,
`SELECT id, active, name, crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantInfo', info, true) FROM system.tenants`,
[][]string{
{`1`, `true`, `system`, `{"id": "1", "name": "system", "state": "ACTIVE"}`},
{`10`, `true`, `NULL`, `{"id": "10", "name": "", "state": "ACTIVE"}`},
{`1`, `true`, `system`, `{"droppedName": "", "id": "1", "name": "system", "state": "ACTIVE"}`},
{`10`, `true`, `NULL`, `{"droppedName": "", "id": "10", "name": "", "state": "ACTIVE"}`},
},
)
restoreDB.CheckQueryResults(t,
Expand Down Expand Up @@ -6858,8 +6858,8 @@ func TestBackupRestoreTenant(t *testing.T) {
restoreDB.CheckQueryResults(t,
`select id, active, name, crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantInfo', info, true) from system.tenants`,
[][]string{
{`1`, `true`, `system`, `{"id": "1", "name": "system", "state": "ACTIVE"}`},
{`10`, `false`, `NULL`, `{"id": "10", "name": "", "state": "DROP"}`},
{`1`, `true`, `system`, `{"droppedName": "", "id": "1", "name": "system", "state": "ACTIVE"}`},
{`10`, `false`, `NULL`, `{"droppedName": "", "id": "10", "name": "", "state": "DROP"}`},
},
)

Expand All @@ -6883,8 +6883,8 @@ func TestBackupRestoreTenant(t *testing.T) {
restoreDB.CheckQueryResults(t,
`select id, active, name, crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantInfo', info, true) from system.tenants`,
[][]string{
{`1`, `true`, `system`, `{"id": "1", "name": "system", "state": "ACTIVE"}`},
{`10`, `true`, `NULL`, `{"id": "10", "name": "", "state": "ACTIVE"}`},
{`1`, `true`, `system`, `{"droppedName": "", "id": "1", "name": "system", "state": "ACTIVE"}`},
{`10`, `true`, `NULL`, `{"droppedName": "", "id": "10", "name": "", "state": "ACTIVE"}`},
},
)

Expand All @@ -6908,14 +6908,14 @@ func TestBackupRestoreTenant(t *testing.T) {
restoreDB.CheckQueryResults(t,
`select id, active, name, crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantInfo', info, true) from system.tenants`,
[][]string{
{`1`, `true`, `system`, `{"id": "1", "name": "system", "state": "ACTIVE"}`},
{`1`, `true`, `system`, `{"droppedName": "", "id": "1", "name": "system", "state": "ACTIVE"}`},
})
restoreDB.Exec(t, `RESTORE TENANT 10 FROM 'nodelocal://1/t10'`)
restoreDB.CheckQueryResults(t,
`select id, active, name, crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantInfo', info, true) from system.tenants`,
[][]string{
{`1`, `true`, `system`, `{"id": "1", "name": "system", "state": "ACTIVE"}`},
{`10`, `true`, `NULL`, `{"id": "10", "name": "", "state": "ACTIVE"}`},
{`1`, `true`, `system`, `{"droppedName": "", "id": "1", "name": "system", "state": "ACTIVE"}`},
{`10`, `true`, `NULL`, `{"droppedName": "", "id": "10", "name": "", "state": "ACTIVE"}`},
},
)
})
Expand All @@ -6937,14 +6937,14 @@ func TestBackupRestoreTenant(t *testing.T) {
restoreDB.CheckQueryResults(t,
`select id, active, name, crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantInfo', info, true) from system.tenants`,
[][]string{
{`1`, `true`, `system`, `{"id": "1", "name": "system", "state": "ACTIVE"}`},
{`1`, `true`, `system`, `{"droppedName": "", "id": "1", "name": "system", "state": "ACTIVE"}`},
})
restoreDB.Exec(t, `RESTORE TENANT 10 FROM 'nodelocal://1/clusterwide'`)
restoreDB.CheckQueryResults(t,
`select id, active, name, crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantInfo', info, true) from system.tenants`,
[][]string{
{`1`, `true`, `system`, `{"id": "1", "name": "system", "state": "ACTIVE"}`},
{`10`, `true`, `NULL`, `{"id": "10", "name": "", "state": "ACTIVE"}`},
{`1`, `true`, `system`, `{"droppedName": "", "id": "1", "name": "system", "state": "ACTIVE"}`},
{`10`, `true`, `NULL`, `{"droppedName": "", "id": "10", "name": "", "state": "ACTIVE"}`},
},
)

Expand Down Expand Up @@ -6977,16 +6977,16 @@ func TestBackupRestoreTenant(t *testing.T) {
restoreDB.CheckQueryResults(t,
`select id, active, name, crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantInfo', info, true) from system.tenants`,
[][]string{
{`1`, `true`, `system`, `{"id": "1", "name": "system", "state": "ACTIVE"}`},
{`1`, `true`, `system`, `{"droppedName": "", "id": "1", "name": "system", "state": "ACTIVE"}`},
})
restoreDB.Exec(t, `RESTORE FROM 'nodelocal://1/clusterwide'`)
restoreDB.CheckQueryResults(t,
`select id, active, name, crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantInfo', info, true) from system.tenants`,
[][]string{
{`1`, `true`, `system`, `{"id": "1", "name": "system", "state": "ACTIVE"}`},
{`10`, `true`, `NULL`, `{"id": "10", "name": "", "state": "ACTIVE"}`},
{`11`, `true`, `NULL`, `{"id": "11", "name": "", "state": "ACTIVE"}`},
{`20`, `true`, `NULL`, `{"id": "20", "name": "", "state": "ACTIVE"}`},
{`1`, `true`, `system`, `{"droppedName": "", "id": "1", "name": "system", "state": "ACTIVE"}`},
{`10`, `true`, `NULL`, `{"droppedName": "", "id": "10", "name": "", "state": "ACTIVE"}`},
{`11`, `true`, `NULL`, `{"droppedName": "", "id": "11", "name": "", "state": "ACTIVE"}`},
{`20`, `true`, `NULL`, `{"droppedName": "", "id": "20", "name": "", "state": "ACTIVE"}`},
},
)

Expand Down
18 changes: 9 additions & 9 deletions pkg/ccl/backupccl/testdata/backup-restore/encrypted-backups
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,21 +170,21 @@ RESTORE DATABASE d FROM LATEST IN 'nodelocal://1/full3' WITH new_db_name='d4', k

query-sql
USE d2;
SELECT * FROM [SHOW TABLES] ORDER BY table_name;
SELECT table_name FROM [SHOW TABLES] ORDER BY table_name;
----
public baz table root <nil> <nil>
public foo table root <nil> <nil>
baz
foo

query-sql
USE d3;
SELECT * FROM [SHOW TABLES] ORDER BY table_name;
SELECT table_name FROM [SHOW TABLES] ORDER BY table_name;
----
public baz table root <nil> <nil>
public foo table root <nil> <nil>
baz
foo

query-sql
USE d4;
SELECT * FROM [SHOW TABLES] ORDER BY table_name;
SELECT table_name FROM [SHOW TABLES] ORDER BY table_name;
----
public baz table root <nil> <nil>
public foo table root <nil> <nil>
baz
foo
20 changes: 10 additions & 10 deletions pkg/ccl/backupccl/testdata/backup-restore/restore-tenants
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ SELECT crdb_internal.destroy_tenant(5);
query-sql
SELECT id,active,crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantInfo', info, true) FROM system.tenants;
----
1 true {"id": "1", "name": "system", "state": "ACTIVE"}
5 false {"id": "5", "name": "", "state": "DROP"}
6 true {"id": "6", "name": "", "state": "ACTIVE"}
1 true {"droppedName": "", "id": "1", "name": "system", "state": "ACTIVE"}
5 false {"droppedName": "", "id": "5", "name": "", "state": "DROP"}
6 true {"droppedName": "", "id": "6", "name": "", "state": "ACTIVE"}

exec-sql
BACKUP INTO 'nodelocal://1/cluster'
Expand All @@ -49,9 +49,9 @@ RESTORE FROM LATEST IN 'nodelocal://1/cluster'
query-sql
SELECT id,active,crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantInfo', info, true) FROM system.tenants;
----
1 true {"id": "1", "name": "system", "state": "ACTIVE"}
5 false {"id": "5", "name": "", "state": "DROP"}
6 true {"id": "6", "name": "", "state": "ACTIVE"}
1 true {"droppedName": "", "id": "1", "name": "system", "state": "ACTIVE"}
5 false {"droppedName": "", "id": "5", "name": "", "state": "DROP"}
6 true {"droppedName": "", "id": "6", "name": "", "state": "ACTIVE"}

exec-sql
RESTORE TENANT 6 FROM LATEST IN 'nodelocal://1/tenant6' WITH tenant = '7';
Expand All @@ -60,7 +60,7 @@ RESTORE TENANT 6 FROM LATEST IN 'nodelocal://1/tenant6' WITH tenant = '7';
query-sql
SELECT id,active,crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantInfo', info, true) FROM system.tenants;
----
1 true {"id": "1", "name": "system", "state": "ACTIVE"}
5 false {"id": "5", "name": "", "state": "DROP"}
6 true {"id": "6", "name": "", "state": "ACTIVE"}
7 true {"id": "7", "name": "", "state": "ACTIVE"}
1 true {"droppedName": "", "id": "1", "name": "system", "state": "ACTIVE"}
5 false {"droppedName": "", "id": "5", "name": "", "state": "DROP"}
6 true {"droppedName": "", "id": "6", "name": "", "state": "ACTIVE"}
7 true {"droppedName": "", "id": "7", "name": "", "state": "ACTIVE"}
5 changes: 4 additions & 1 deletion pkg/sql/catalog/descpb/tenant.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ message TenantInfo {
optional string name = 3 [
(gogoproto.nullable) = false,
(gogoproto.customtype) = "github.com/cockroachdb/cockroach/pkg/roachpb.TenantName"];
optional string dropped_name = 4 [
(gogoproto.nullable) = false,
(gogoproto.customtype) = "github.com/cockroachdb/cockroach/pkg/roachpb.TenantName"];
}

// TenantInfoAndUsage contains the information for a tenant in a multi-tenant
Expand All @@ -53,7 +56,7 @@ message TenantInfoWithUsage {
optional double ru_burst_limit = 1 [(gogoproto.nullable) = false, (gogoproto.customname) = "RUBurstLimit"];
optional double ru_refill_rate = 2 [(gogoproto.nullable) = false, (gogoproto.customname) = "RURefillRate"];
optional double ru_current = 3 [(gogoproto.nullable) = false, (gogoproto.customname) = "RUCurrent"];

// All-time consumption for this tenant. Each field has a corresponding column
// in system.tenant_usage.
optional roachpb.TenantConsumption consumption = 4 [(gogoproto.nullable) = false];
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/create_tenant.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ type createTenantNode struct {

func (p *planner) CreateTenantNode(_ context.Context, n *tree.CreateTenant) (planNode, error) {
return &createTenantNode{
name: roachpb.TenantName(n.Name.String()),
name: roachpb.TenantName(n.Name),
}, nil
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/drop_tenant.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ type dropTenantNode struct {

func (p *planner) DropTenant(_ context.Context, n *tree.DropTenant) (planNode, error) {
return &dropTenantNode{
name: roachpb.TenantName(n.Name.String()),
name: roachpb.TenantName(n.Name),
ifExists: n.IfExists,
}, nil
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/gcjob_test/gc_job_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,7 @@ func TestGCTenant(t *testing.T) {
require.EqualError(
t,
gcClosure(dropTenID, progress),
`GC state for tenant id:11 state:DROP name:"" is DELETED yet the tenant row still exists`,
`GC state for tenant id:11 state:DROP name:"" dropped_name:"" is DELETED yet the tenant row still exists`,
)
})

Expand Down
47 changes: 37 additions & 10 deletions pkg/sql/logictest/testdata/logic_test/tenant
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ query IBIT colnames
SELECT id, active, length(info), name FROM system.tenants ORDER BY id
----
id active length name
1 true 12 system
1 true 14 system

# Create a few tenants.

Expand All @@ -21,11 +21,11 @@ SELECT id, active, name, crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantI
FROM system.tenants
ORDER BY id
----
id active name crdb_internal.pb_to_json
1 true system {"id": "1", "name": "system", "state": "ACTIVE"}
2 true "tenant-one" {"id": "2", "name": "\"tenant-one\"", "state": "ACTIVE"}
3 true two {"id": "3", "name": "two", "state": "ACTIVE"}
4 true three {"id": "4", "name": "three", "state": "ACTIVE"}
id active name crdb_internal.pb_to_json
1 true system {"droppedName": "", "id": "1", "name": "system", "state": "ACTIVE"}
2 true tenant-one {"droppedName": "", "id": "2", "name": "tenant-one", "state": "ACTIVE"}
3 true two {"droppedName": "", "id": "3", "name": "two", "state": "ACTIVE"}
4 true three {"droppedName": "", "id": "4", "name": "three", "state": "ACTIVE"}

# Test creating a tenant with the same name as an existing tenant, but a unique
# ID.
Expand Down Expand Up @@ -63,8 +63,8 @@ SELECT id, active, name, crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantI
FROM system.tenants WHERE name = 'four'
ORDER BY id
----
id active name crdb_internal.pb_to_json
5 true four {"id": "5", "name": "four", "state": "ACTIVE"}
id active name crdb_internal.pb_to_json
5 true four {"droppedName": "", "id": "5", "name": "four", "state": "ACTIVE"}

statement ok
DROP TENANT four
Expand All @@ -74,8 +74,7 @@ SELECT id, active, name, crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantI
FROM system.tenants WHERE name = 'four'
ORDER BY id
----
id active name crdb_internal.pb_to_json
5 false four {"id": "5", "name": "four", "state": "DROP"}
id active name crdb_internal.pb_to_json

statement ok
CREATE TENANT "five-requiring-quotes"
Expand All @@ -96,3 +95,31 @@ user testuser

statement error only users with the admin role are allowed to destroy tenant
DROP TENANT "not-allowed"

user root

subtest reclaim_name

statement ok
CREATE TENANT "to-be-reclaimed"

statement ok
DROP TENANT "to-be-reclaimed"

statement ok
CREATE TENANT "to-be-reclaimed"

query IBTT colnames
SELECT id, active, name, crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantInfo', info, true)
FROM system.tenants
ORDER BY id
----
id active name crdb_internal.pb_to_json
1 true system {"droppedName": "", "id": "1", "name": "system", "state": "ACTIVE"}
2 true tenant-one {"droppedName": "", "id": "2", "name": "tenant-one", "state": "ACTIVE"}
3 true two {"droppedName": "", "id": "3", "name": "two", "state": "ACTIVE"}
4 true three {"droppedName": "", "id": "4", "name": "three", "state": "ACTIVE"}
5 false NULL {"droppedName": "four", "id": "5", "name": "", "state": "DROP"}
6 false NULL {"droppedName": "five-requiring-quotes", "id": "6", "name": "", "state": "DROP"}
7 false NULL {"droppedName": "to-be-reclaimed", "id": "7", "name": "", "state": "DROP"}
8 true to-be-reclaimed {"droppedName": "", "id": "8", "name": "to-be-reclaimed", "state": "ACTIVE"}
24 changes: 12 additions & 12 deletions pkg/sql/logictest/testdata/logic_test/tenant_builtins
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ query IBIT colnames
SELECT id, active, length(info), name FROM system.tenants ORDER BY id
----
id active length name
1 true 12 system
1 true 14 system

# Create three tenants.

Expand All @@ -28,10 +28,10 @@ FROM system.tenants
ORDER BY id
----
id active name crdb_internal.pb_to_json
1 true system {"id": "1", "name": "system", "state": "ACTIVE"}
2 true tenant-number-eleven {"id": "2", "name": "tenant-number-eleven", "state": "ACTIVE"}
5 true NULL {"id": "5", "name": "", "state": "ACTIVE"}
10 true tenant-number-ten {"id": "10", "name": "tenant-number-ten", "state": "ACTIVE"}
1 true system {"droppedName": "", "id": "1", "name": "system", "state": "ACTIVE"}
2 true tenant-number-eleven {"droppedName": "", "id": "2", "name": "tenant-number-eleven", "state": "ACTIVE"}
5 true NULL {"droppedName": "", "id": "5", "name": "", "state": "ACTIVE"}
10 true tenant-number-ten {"droppedName": "", "id": "10", "name": "tenant-number-ten", "state": "ACTIVE"}

# Check we can add a name where none existed before.
query I
Expand Down Expand Up @@ -89,10 +89,10 @@ FROM system.tenants
ORDER BY id
----
id active name crdb_internal.pb_to_json
1 true system {"id": "1", "name": "system", "state": "ACTIVE"}
2 true tenant-number-eleven {"id": "2", "name": "tenant-number-eleven", "state": "ACTIVE"}
5 false my-new-tenant-name {"id": "5", "name": "my-new-tenant-name", "state": "DROP"}
10 true tenant-number-ten {"id": "10", "name": "tenant-number-ten", "state": "ACTIVE"}
1 true system {"droppedName": "", "id": "1", "name": "system", "state": "ACTIVE"}
2 true tenant-number-eleven {"droppedName": "", "id": "2", "name": "tenant-number-eleven", "state": "ACTIVE"}
5 false NULL {"droppedName": "my-new-tenant-name", "id": "5", "name": "", "state": "DROP"}
10 true tenant-number-ten {"droppedName": "", "id": "10", "name": "tenant-number-ten", "state": "ACTIVE"}


# Try to recreate an existing tenant.
Expand Down Expand Up @@ -199,9 +199,9 @@ FROM system.tenants
ORDER BY id
----
id active crdb_internal.pb_to_json
1 true {"id": "1", "name": "system", "state": "ACTIVE"}
2 true {"id": "2", "name": "tenant-number-eleven", "state": "ACTIVE"}
10 true {"id": "10", "name": "tenant-number-ten", "state": "ACTIVE"}
1 true {"droppedName": "", "id": "1", "name": "system", "state": "ACTIVE"}
2 true {"droppedName": "", "id": "2", "name": "tenant-number-eleven", "state": "ACTIVE"}
10 true {"droppedName": "", "id": "10", "name": "tenant-number-ten", "state": "ACTIVE"}

query error tenant resource limits require a CCL binary
SELECT crdb_internal.update_tenant_resource_limits(10, 1000, 100, 0, now(), 0)
Expand Down
2 changes: 2 additions & 0 deletions pkg/sql/tenant.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,8 @@ func destroyTenantInternal(
// TODO(ssd): We may want to implement a job that waits out
// any running sql pods before enqueing the GC job.
info.State = descpb.TenantInfo_DROP
info.DroppedName = info.Name
info.Name = ""
if err := updateTenantRecord(ctx, execCfg, txn, info); err != nil {
return errors.Wrap(err, "destroying tenant")
}
Expand Down

0 comments on commit 98197fd

Please sign in to comment.