sql: support tenant creation and deletion, track in system.tenants table

[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.

[cockroach-teamcity]

This change is 

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on e32ad865.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[knz]

would a JSONB field for info not be more useful?

[asubiotto]

I've looked at #48513 and I'm not sure I understand the thinking behind having a single DescIDSequence for all tenants. Why is it not better to have one per tenant and stored in tenant keyspace?

Reviewed 10 of 10 files at r1, 3 of 3 files at r2, 6 of 6 files at r3, 17 of 17 files at r4, 3 of 3 files at r5, 4 of 4 files at r6, 29 of 29 files at r7.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @tbg)

[tbg]

@asubiotto isn't the suggestion that each tenant uses a sequence? It's not sharing the sequence between tenants. So I think he's suggesting what you're suggesting, perhaps ambiguously framed.

[tbg]

(I did not look at first three commits). I assume you'll want to wait for @asubiotto's review though.

Reviewed 10 of 10 files at r1, 3 of 3 files at r2, 6 of 6 files at r3, 17 of 17 files at r4, 3 of 3 files at r5, 4 of 4 files at r6, 29 of 29 files at r7.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

---

docs/generated/sql/functions.md, line 1061 at r7 (raw file):

</table>

### Multi-tenancy functions

Is this internal documentation? We'll want to avoid leaking anything about these into the official docs.

---

pkg/sql/descriptor.go, line 60 at r4 (raw file):

func GenerateUniqueDescID(ctx context.Context, db *kv.DB) (sqlbase.ID, error) {
	// Increment unique descriptor counter.
	newVal, err := kv.IncrementValRetryable(ctx, db, keys.DescIDGenerator, 1)

Can DescIDGenerator be unexported?

---

pkg/sql/tenant.go, line 67 at r7 (raw file):

		keys.MakeSQLCodec(roachpb.MakeTenantID(tenID)),
		zonepb.DefaultZoneConfigRef(),
		zonepb.DefaultSystemZoneConfigRef(),

These are unused, right? Can we avoid supplying them?

---

pkg/sql/tenant.go, line 100 at r7 (raw file):

		// NOTE: even if we got this wrong, the rest of the function would fail
		// for a non-system tenant because they would be missing a system.tenant
		// table.

It would (at some point) also fail because the KV layer won't let them.

---

pkg/sql/tenant.go, line 129 at r7 (raw file):

	// TODO(nvanbenschoten): actually clear tenant keyspace. Do we want to do
	// this synchronously or schedule it as a long-running job. Probably the

Definitely not in the same transaction. I would say active = false is irrevocable and we start ClearRanging one the txn commits (blocking return of destroy_tenant). If that fails too early, can just re-run the command. The // tenant already destroyed case would go away.

Are you intentionally keeping inactive tenants around? I thought they'd be gone once they're gone, but could be recreated.

---

pkg/sql/logictest/testdata/logic_test/grant_table, line 168 at r7 (raw file):

SELECT * FROM [SHOW GRANTS]
 WHERE schema_name NOT IN ('crdb_internal', 'pg_catalog', 'information_schema')
ORDER BY 1,2,3

Why the large diff in this method? Is that just bad diffing?

---

pkg/sql/sqlbase/metadata.go, line 231 at r5 (raw file):

	// this cache for system descriptors that are only found in non-system
	// tenants will not be found. That is ok for now, but we'll need to split
	// this cache or do something smarter should it become a problem.

I would rather fix this right now before we forget about it. The only caller into this cache is

cockroach/pkg/sql/physical_schema_accessors.go

Line 204 in c08088d

descID := sqlbase.LookupSystemTableDescriptorID(ctx, settings, dbID, object)

which has the codec available. So you can just pre-generate two maps here, and index into the right one. The only plumbing is passing the codec one layer down into UncachedPhysicalAccessor.GetObjectDesc, and that only affects eight call sites.

---

pkg/sql/sqlbase/system.go, line 108 at r7 (raw file):

	id     INT8 NOT NULL PRIMARY KEY,
	active BOOL NOT NULL DEFAULT true,
	info   BYTES,

It seems unclear what we ultimately want here. I'm fine leaving this info field in, but let's add a comment that it's just a placeholder and may be kept, replaced, or even just removed.

[nvanbenschoten]

TFTRs!

would a JSONB field for info not be more useful?

I was thinking that we might want to store encoded protobufs in the field as well, so I think it's best to keep the data type more general. It's easy enough to go from JSON to BYTES and back.

isn't the suggestion that each tenant uses a sequence? It's not sharing the sequence between tenants. So I think he's suggesting what you're suggesting, perhaps ambiguously framed.

Right, #48513 wasn't intending to mean that we use a "single SQL sequence as DescIDGenerator for all non-system tenants", it was intending to mean that we use a "SQL sequence as DescIDGenerator for each non-system tenants". I can see how that was ambiguous though.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @tbg)

---

docs/generated/sql/functions.md, line 1061 at r7 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Is this internal documentation? We'll want to avoid leaking anything about these into the official docs.

Done.

---

pkg/sql/descriptor.go, line 60 at r4 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Can DescIDGenerator be unexported?

Done.

---

pkg/sql/tenant.go, line 67 at r7 (raw file):

Previously, tbg (Tobias Grieger) wrote…

These are unused, right? Can we avoid supplying them?

Done by removing the system.zones table entirely from secondary tenants.

---

pkg/sql/tenant.go, line 100 at r7 (raw file):

Previously, tbg (Tobias Grieger) wrote…

It would (at some point) also fail because the KV layer won't let them.

Good point. KV level verification of tenant key accesses is going to be a great way to flush out bugs.

---

pkg/sql/tenant.go, line 129 at r7 (raw file):

Definitely not in the same transaction. I would say active = false is irrevocable and we start ClearRanging one the txn commits (blocking return of destroy_tenant). If that fails too early, can just re-run the command. The // tenant already destroyed case would go away.

I like that. I updated #48775 and the comment with the idea.

Are you intentionally keeping inactive tenants around? I thought they'd be gone once they're gone, but could be recreated.

No, I'm not intentionally keeping them around, I just figured that a soft delete would give us more flexibility until we decide on exactly how we want to implement space reclamation.

---

pkg/sql/logictest/testdata/logic_test/grant_table, line 168 at r7 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Why the large diff in this method? Is that just bad diffing?

It looks like rowsort was hiding the fact that this output was in the completely wrong order (ORDER BY 1,2,3). Oliver also hit this in 86641f9#diff-b254deb01f059b658ce2401c034f3bdfL169.

I've removed the rowsort because it's not needed with the explicit ordering.

---

pkg/sql/sqlbase/metadata.go, line 231 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

I would rather fix this right now before we forget about it. The only caller into this cache is

cockroach/pkg/sql/physical_schema_accessors.go

Line 204 in c08088d

descID := sqlbase.LookupSystemTableDescriptorID(ctx, settings, dbID, object)

which has the codec available. So you can just pre-generate two maps here, and index into the right one. The only plumbing is passing the codec one layer down into UncachedPhysicalAccessor.GetObjectDesc, and that only affects eight call sites.

Done.

---

pkg/sql/sqlbase/system.go, line 108 at r7 (raw file):

Previously, tbg (Tobias Grieger) wrote…

It seems unclear what we ultimately want here. I'm fine leaving this info field in, but let's add a comment that it's just a placeholder and may be kept, replaced, or even just removed.

Done.

[nvanbenschoten]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)