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.

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

sql: tenant deletion #47904

Closed
tbg opened this issue Apr 22, 2020 · 2 comments
Closed

sql: tenant deletion #47904

tbg opened this issue Apr 22, 2020 · 2 comments
Assignees
Labels
A-multitenancy Related to multi-tenancy

Comments

@tbg
Copy link
Member

tbg commented Apr 22, 2020

Add a builtin creating/deleting the keyspace for a given tenant ID as outlined in the design doc as well as an internal (zero-tenant) metadata table that keeps track of which tenants exist.

@tbg tbg added A-multitenancy Related to multi-tenancy A-multitenancy-blocker labels Apr 22, 2020
@tbg tbg changed the title sql: seed tenant keyspace sql: tenant creation/deletion and tracking Apr 22, 2020
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue May 4, 2020
Informs cockroachdb#48123.

This commit continues with the plumbing that began an cockroachdb#48190. It pushes
a tenant-bound SQL codec into the other main source of key generation in
the SQL layer - descriptor manipulation and metadata handling. This
allows SQL tenants to properly handle metadata descriptors for its
database and tables.

This ended up being a larger undertaking than I had originally expected.
However, now that it's complete, we're in a pretty good spot:
1. `sqlbase.MetadataSchema` is ready to be used for cockroachdb#47904.
2. we can now run SQL migrations for a non-system tenant
3. there is only one remaining use of TODOSQLCodec in pkg/sql. See cockroachdb#48375.
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue May 5, 2020
Informs cockroachdb#48123.

This commit continues with the plumbing that began an cockroachdb#48190. It pushes
a tenant-bound SQL codec into the other main source of key generation in
the SQL layer - descriptor manipulation and metadata handling. This
allows SQL tenants to properly handle metadata descriptors for its
database and tables.

This ended up being a larger undertaking than I had originally expected.
However, now that it's complete, we're in a pretty good spot:
1. `sqlbase.MetadataSchema` is ready to be used for cockroachdb#47904.
2. we can now run SQL migrations for a non-system tenant
3. there is only one remaining use of TODOSQLCodec in pkg/sql. See cockroachdb#48375.
craig bot pushed a commit that referenced this issue May 5, 2020
48376: sql: push tenant-bound SQL codec into descriptor key generation r=nvanbenschoten a=nvanbenschoten

Informs #48123.

This commit continues with the plumbing that began an #48190. It pushes a tenant-bound SQL codec into the other main source of key generation in the SQL layer - descriptor manipulation and metadata handling. This allows SQL tenants to properly handle metadata descriptors for its database and tables.

This ended up being a larger undertaking than I had originally expected. However, now that it's complete, we're in a pretty good spot:
1. `sqlbase.MetadataSchema` is ready to be used for #47904.
2. we can now run SQL migrations for a non-system tenant.
3. there is only one remaining use of `keys.TODOSQLCodec` in pkg/sql. See #48375.

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Anzoteh96 pushed a commit to Anzoteh96/cockroach that referenced this issue May 5, 2020
48376: sql: push tenant-bound SQL codec into descriptor key generation r=nvanbenschoten a=nvanbenschoten

Informs cockroachdb#48123.

This commit continues with the plumbing that began an cockroachdb#48190. It pushes a tenant-bound SQL codec into the other main source of key generation in the SQL layer - descriptor manipulation and metadata handling. This allows SQL tenants to properly handle metadata descriptors for its database and tables.

This ended up being a larger undertaking than I had originally expected. However, now that it's complete, we're in a pretty good spot:
1. `sqlbase.MetadataSchema` is ready to be used for cockroachdb#47904.
2. we can now run SQL migrations for a non-system tenant.
3. there is only one remaining use of `keys.TODOSQLCodec` in pkg/sql. See cockroachdb#48375.

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>

Release note (<category, see below>): <what> <show> <why>
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue May 6, 2020
Addresses a TODO and helps clear the way for cockroachdb#47904.
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue May 13, 2020
Addresses a TODO and helps clear the way for cockroachdb#47904.
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue May 13, 2020
Most of cockroachdb#47904.

First commit from cockroachdb#48504.
Second and third commit from cockroachdb#48773.

This commit implements the initial structure for tenant creation and
deletion. It does so by introducing a new system table to track tenants
in a multi-tenant cluster and two new builtin functions to manipulate
this table and the overall multi-tenant state.

The change starts by introducing a new `system.tenants` table with the
following schema:
```
CREATE TABLE system.tenants (
    id     INT8 NOT NULL PRIMARY KEY,
    active BOOL NOT NULL DEFAULT true,
    info   BYTES
);
```

The `id` column is self-explanatory. The `active` column is used to
coordinate tenant destruction in an asynchronous job. The `info` column
is an opaque byte slice to allow users to associate arbitrary
information with specific tenants. I don't know exactly how this third
field will be used (mapping tenants back to CockroachCloud user IDs?),
but it seems like a good idea to add some flexibility since we do intend
to eventually expose this externally.

The table is given an ID of 8, which means that it falls within the
"system config span". I believe this is ok, because the entire concept
of the "system config span" should be going away with cockroachdb#47150. The table
is also only exposed to the system-tenant and is never created for
secondary tenants.

The change then introduces two new builtin functions:
`crdb_internal.create_tenant` and `crdb_internal.destroy_tenant`. These
do what you would expect - creating and destroying tenant keyspaces,
along with updating metadata in system.tenants.
craig bot pushed a commit that referenced this issue May 13, 2020
48504: sql: Remove cluster version input to GetInitialValues r=nvanbenschoten a=nvanbenschoten

Addresses a TODO and helps clear the way for #47904.

This does not remove the rest of the references to `VersionNamespaceTableWithSchemas`.

48773: sql: properly handle NULL hashedPassword column in system.users r=nvanbenschoten a=nvanbenschoten

Fixes #48769.

Before this change, attempts to log in as a user with a NULL value in their "hashedPassword" column in the `system.users` table would cause the server to crash. This was because `retrieveUserAndPassword` was not properly handling NULL values.

This change fixes this. It then fixes the bug that led me to the first one - we were not properly handling nil byte slices in `golangFillQueryArguments`. Nil byte slices were being treated as empty byte slices, which resulted in confusing behavior where a nil byte slice passed to an InternalExecutor would not be NULL. I'm considered backporting this fix, but I don't think we should. I fear that doing so will have unintended consequences by tickling other bugs like what is fixed in the first commit.

Finally, this fixes `TestGolangQueryArgs`, which was completely broken and asserting that `reflect.Type(*types.T) == reflect.Type(*types.T)`.

Release note (bug fix): Manually writing a NULL value into the system.users table for the "hashedPassword" column will no longer cause a server crash during user authentication.

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue May 14, 2020
Most of cockroachdb#47904.

First commit from cockroachdb#48504.
Second and third commit from cockroachdb#48773.

This commit implements the initial structure for tenant creation and
deletion. It does so by introducing a new system table to track tenants
in a multi-tenant cluster and two new builtin functions to manipulate
this table and the overall multi-tenant state.

The change starts by introducing a new `system.tenants` table with the
following schema:
```
CREATE TABLE system.tenants (
    id     INT8 NOT NULL PRIMARY KEY,
    active BOOL NOT NULL DEFAULT true,
    info   BYTES
);
```

The `id` column is self-explanatory. The `active` column is used to
coordinate tenant destruction in an asynchronous job. The `info` column
is an opaque byte slice to allow users to associate arbitrary
information with specific tenants. I don't know exactly how this third
field will be used (mapping tenants back to CockroachCloud user IDs?),
but it seems like a good idea to add some flexibility since we do intend
to eventually expose this externally.

The table is given an ID of 8, which means that it falls within the
"system config span". I believe this is ok, because the entire concept
of the "system config span" should be going away with cockroachdb#47150. The table
is also only exposed to the system-tenant and is never created for
secondary tenants.

The change then introduces two new builtin functions:
`crdb_internal.create_tenant` and `crdb_internal.destroy_tenant`. These
do what you would expect - creating and destroying tenant keyspaces,
along with updating metadata in system.tenants.
craig bot pushed a commit that referenced this issue May 14, 2020
48778: sql: support tenant creation and deletion, track in system.tenants table r=nvanbenschoten a=nvanbenschoten

Most of #47904.

This commit implements the initial structure for tenant creation and deletion. It does so by introducing a new system table to track tenants in a multi-tenant cluster and two new builtin functions to manipulate this table and the overall multi-tenant state.

The change starts by introducing a new `system.tenants` table with the following schema:
```
CREATE TABLE system.tenants (
    id     INT8 NOT NULL PRIMARY KEY,
    active BOOL NOT NULL DEFAULT true,
    info   BYTES
);
```

The `id` column is self-explanatory. The `active` column is used to coordinate tenant destruction in an asynchronous job. The `info` column is an opaque byte slice to allow users to associate arbitrary information with specific tenants. I don't know exactly how this third field will be used (mapping tenants back to CockroachCloud user IDs?), but it seems like a good idea to add some flexibility since we do intend to eventually expose this externally.

The table is given an ID of 8, which means that it falls within the "system config span". I believe this is ok, because the entire concept of the "system config span" should be going away with #47150. The table is also only exposed to the system-tenant and is never created for secondary tenants.

The change then introduces two new builtin functions: `crdb_internal.create_tenant` and `crdb_internal.destroy_tenant`. These do what you would expect - creating and destroying tenant keyspaces, along with updating metadata in system.tenants.

48830: opt: correctly ignore NULL in array indirection r=mjibson a=mjibson

Fixes #48826

Release note (bug fix): fix an error that could occur when using NULL
in some array indirections.

49081: sql: fix bug where columns couldn't be dropped after a pk change r=rohany a=rohany

Fixes #49079.

Release note (bug fix): Fix a bug where columns of a table could not
be dropped after a primary key change.

49082: kv: remove special handling of MergeTrigger in batcheval r=nvanbenschoten a=nvanbenschoten

Discovered with @ajwerner while investigating #48770.

This commit removes the special treatment that we used to give
the MergeTrigger - running it before local lock cleanup. This
was originally added back when the MergeTrigger held a snapshot
of the RHS's data, including intents that needed to be resolved
synchronously with the trigger. This snapshot was removed shortly
afterwards in bb7426a, but the special handling during batch
evaluation persisted, leading to confusing code.

While here, avoid copying around the transaction Result on the
hot path where there are no commit triggers. This was not free,
as is demonstrated by the following benchmark:
```
func BenchmarkMergeTxnCommitResult(b *testing.B) {
	locks := []roachpb.Span{{Key: roachpb.Key("a")}, {Key: roachpb.Key("b")}}
	txn := &roachpb.Transaction{LockSpans: locks}
	txnResult := result.FromEndTxn(txn, false /* alwaysReturn */, false)
	txnResult.Local.UpdatedTxns = []*roachpb.Transaction{txn}
	txnResult.Local.ResolvedLocks = roachpb.AsLockUpdates(txn, locks)

	for i := 0; i < b.N; i++ {
		var res result.Result
		if err := res.MergeAndDestroy(txnResult); err != nil {
			b.Fatal(err)
		}
	}
}

// BenchmarkMergeTxnCommitResult-16   12777922   86.2 ns/op   0 B/op   0 allocs/op
```

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Matt Jibson <matt.jibson@gmail.com>
Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
@tbg
Copy link
Member Author

tbg commented Jul 8, 2020

What's left here is tenant deletion, right?

@tbg tbg changed the title sql: tenant creation/deletion and tracking sql: tenant deletion Jul 8, 2020
@nvanbenschoten
Copy link
Member

Yes. I marked #48775 as a blocker and am folding this into it.

spaskob pushed a commit to spaskob/cockroach that referenced this issue Oct 27, 2020
Informs cockroachdb#47904.

This is a stop gap solution that will perform the operation
synchronously inside the transaction that marks the tenant row
as DROP and finally deletes the row.

This PR is intended to be backported to 20.2 release branch.
The long-term solution that implements cockroachdb#48775 is done in cockroachdb#55756
and will be our long term approach for release 21.0 and beyond.

Release note: none.
spaskob pushed a commit to spaskob/cockroach that referenced this issue Oct 28, 2020
Informs cockroachdb#47904.

This is a stop gap solution that will perform the operation
synchronously inside the transaction that marks the tenant row
as DROP and finally deletes the row.

This PR is intended to be backported to 20.2 release branch.
The long-term solution that implements cockroachdb#48775 is done in cockroachdb#55756
and will be our long term approach for release 21.0 and beyond.

Release note: none.
spaskob pushed a commit to spaskob/cockroach that referenced this issue Oct 28, 2020
Informs cockroachdb#47904.

This is a stop gap solution that will perform the operation
synchronously inside the transaction that marks the tenant row
as DROP and finally deletes the row.

This PR is intended to be backported to 20.2 release branch.
The long-term solution that implements cockroachdb#48775 is done in cockroachdb#55756
and will be our long term approach for release 21.0 and beyond.

Release note: none.
spaskob pushed a commit to spaskob/cockroach that referenced this issue Oct 29, 2020
Informs cockroachdb#47904.

This is a stop gap solution that will perform the operation
synchronously inside the transaction that marks the tenant row
as DROP and finally deletes the row.

This PR is intended to be backported to 20.2 release branch.
The long-term solution that implements cockroachdb#48775 is done in cockroachdb#55756
and will be our long term approach for release 21.0 and beyond.

Release note: none.
craig bot pushed a commit that referenced this issue Oct 29, 2020
55935: sql/tenant: synch clear range data on tenant drop request r=spaskob a=spaskob

Informs #47904.

This is a stop gap solution that will perform the operation
synchronously inside the transaction that marks the tenant row
as DROP and finally deletes the row.

This PR is intended to be backported to 20.2 release branch.
The long-term solution that implements #48775 is done in #55756
and will be our long term approach for release 21.0 and beyond.

Release note: none.

Co-authored-by: Spas Bojanov <spas@cockroachlabs.com>
spaskob pushed a commit to spaskob/cockroach that referenced this issue Oct 29, 2020
Informs cockroachdb#47904.

This is a stop gap solution that will perform the operation
synchronously inside the transaction that marks the tenant row
as DROP and finally deletes the row.

This PR is intended to be backported to 20.2 release branch.
The long-term solution that implements cockroachdb#48775 is done in cockroachdb#55756
and will be our long term approach for release 21.0 and beyond.

Release note: none.
spaskob pushed a commit to spaskob/cockroach that referenced this issue Nov 6, 2020
Informs cockroachdb#47904.

This is a stop gap solution that will perform the operation
synchronously inside the transaction that marks the tenant row
as DROP and finally deletes the row.

This PR is intended to be backported to 20.2 release branch.
The long-term solution that implements cockroachdb#48775 is done in cockroachdb#55756
and will be our long term approach for release 21.0 and beyond.

Release note: none.
spaskob pushed a commit to spaskob/cockroach that referenced this issue Nov 9, 2020
Informs cockroachdb#47904.

This is a stop gap solution that will perform the operation
synchronously inside the transaction that marks the tenant row
as DROP and finally deletes the row.

This PR is intended to be backported to 20.2 release branch.
The long-term solution that implements cockroachdb#48775 is done in cockroachdb#55756
and will be our long term approach for release 21.0 and beyond.

Release note: none.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-multitenancy Related to multi-tenancy
Projects
None yet
Development

No branches or pull requests

2 participants