sql: improve tenant records #95691

knz · 2023-01-23T17:51:03Z

supersedes #95574, #95581, #95655
Epic: CRDB-21836

RFC for context: #96147

TLDR This commit contains the following changes:

rename "state" to "data_state", "active" to "ready"
stored, non-virtual columns for "name", "data_state", "service_mode"
deprecate the column "active" since it mirrors "data_state'
move descpb.TenantInfo to new package mtinfopb.
new statements ALTER TENANT ... START SERVICE EXTERNAL/SHARED,
STOP SERVICE to change the service mode.

Details follow.

rename TenantInfo.State to DataState, "ACTIVE" to "READY"

We've discovered that we'd like to separate the readiness of the data
from the activation of the service. To emphasize this, this commit
renames the field "State" to "DataState".

Additionally, the state "ACTIVE" was confusing as it suggests that
something is running, whereas it merely marks the tenant data as ready
for use. So this commit also renames that state accordingly.

new tenant info field ServiceMode

Summary of changes:

the new TenantInfo.ServiceMode field indicates how to run servers.
new syntax: ALTER TENANT ... START SERVICE EXTERNAL/SHARED,
ALTER TENANT ... STOP SERVICE.
tenants created via create_tenant(<id>)
(via CC serverless control plane) start in service mode EXTERNAL.
other tenants start in service mode NONE.
need ALTER TENANT STOP SERVICE before dropping a tenant.
- except in the case of crdb_internal.destroy_tenant
  for compat with CC serverless control plane.

make the output columns of SHOW TENANT lowercase

All the SHOW statements report status-like data in lowercase.
SHOW TENANT(s) should not be different.

use actual SQL columns for the TenantInfo fields

Release note: None

cockroach-teamcity · 2023-01-23T17:51:13Z

This change is

knz · 2023-01-23T21:59:19Z

I amended the commit to move descpb.TenantInfo to its own package as suggested by Andrew.

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, @herkolategan, @michae2, @smg260, and @stevendanna)

pkg/sql/tenant_update.go line 75 at r13 (raw file):

this does mostly works, but technically is not bulletproof.

I don't fully understand why -- if my reading is correct, the version can only be bumped after the migration is complete, at which point the descs are updated. Where is this understanding incorrect?
we have used this pattern in numerous places before -- use a cluster version gate to choose how to access a system table. If what you say is true, this means we have a wider problem on our hands than just this PR. I'm not sure this PR is the place to fix it.

pkg/upgrade/upgrades/tenant_table_migration.go line 80 at r13 (raw file):

Previously, ajwerner wrote…

The assumption baked into this migration is that no other tenants have names. Is that legit? Maybe comment that somewhere

Yes it is assumed because names can only be introduced after the version field has been updated. Adding comment.

pkg/upgrade/upgrades/tenant_table_migration_test.go line 61 at r13 (raw file):

Previously, ajwerner wrote…

should this test maybe use the tenants table after the upgrade? Perhaps create a tenant and then query the table and validate the structure?

Good idea. Done.

pkg/upgrade/upgrades/tenant_table_migration_test.go line 71 at r13 (raw file):

Previously, ajwerner wrote…

If you take another iteration of the code can you put a TODO here that says we should be bootstrapping into the old version and avoid this dance altogether?

Done.

stevendanna

Overall this looks pretty reasonable to me, but I'd like to see a test or two to encourage us to write test when we make future modifications to this.

I'm not in love with having some fields in the protobuf and some as full columns. But, I don't have a good suggestion here so I defer to the outcome of the previous discussions.

On the topic of reserved 3 in the protobuf. It seems like if we are going to assume that this never shipped by not reserving it, then there isn't much reason to not just do the renumbering now. That is, either we are going to follow the backwards-compatible change rules or not. If we aren't, then let's not leave a gap in the IDs for later.

stevendanna · 2023-01-24T17:19:29Z

pkg/multitenant/mtinfopb/info.go

+	// DataStateReady indicates data is ready and SQL servers can access it.
+	DataStateReady TenantDataState = 0


Do we want the zero-value to be DataStateReady?

pkg/sql/logictest/testdata/logic_test/tenant

knz

I'd like to see a test or two to encourage us to write test when we make future modifications to this.

Done.

I'm not in love with having some fields in the protobuf and some as full columns. But, I don't have a good suggestion here so I defer to the outcome of the previous discussions.

We've discussed elsewhere and I believe the consensus is to push us to go more towards separate columns over time.

On the topic of reserved 3 in the protobuf. It seems like if we are going to assume that this never shipped by not reserving it, then there isn't much reason to not just do the renumbering now.

👍 Done.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, @herkolategan, @michae2, @smg260, and @stevendanna)

pkg/sql/logictest/testdata/logic_test/tenant line 261 at r14 (raw file):

Previously, stevendanna (Steven Danna) wrote…

Can we add some tests that modify the tenant service mode?

Done.

pkg/multitenant/mtinfopb/info.go line 66 at r14 (raw file):

Previously, stevendanna (Steven Danna) wrote…

Do we want the zero-value to be DataStateReady?

I don't know - what do you think? I'm putting it on "Add" now but I think it doesn't matter. We don't use the zero value elsewhere IIRC.

stevendanna

Thanks for adding the tests. I left a few more comments, but I trust you to decide which direction to take them.

I see you already had some discussion with @dt around the shape of the protobufs. It feels a little odd that the main type that most Go code will want to traffic in is TenantInfoExtended. Perhaps just changing the name of that struct to something like TenantRecord or something might make it feel a bit more reasonable.

I think it is likely that this isn't the final form of how we manage/represent the service state of a tenant, but I don't think this will be too hard to change in the future if necessary.

stevendanna · 2023-01-25T21:31:08Z

pkg/sql/parser/sql.y

 | SESSION
 | SESSIONS
 | SET
 | SETS
 | SHARE
+| SHARED


I've recently learned that we should potentially also add these new keywords to the bare_keywords list. But, I plan to do a pass for all the keywords added in 23.1 soon.

stevendanna · 2023-01-25T21:38:29Z

pkg/ccl/backupccl/testdata/backup-restore/restore-tenants

@@ -15,15 +15,15 @@ SELECT crdb_internal.create_tenant(6);

 # Drop one of them.
 exec-sql
-DROP TENANT [5]
+ALTER TENANT [5] STOP SERVICE; DROP TENANT [5]


We could update this test file to show that servce_state is properly restored during a tenant restore.

knz

Perhaps just changing the name of that struct to something like TenantRecord or something might make it feel a bit more reasonable.

Good call. I renamed as follows:

TenantInfo -> ProtoInfo
TenantInfoWithUsage_ExtraColumns -> SQLInfo
ExtendedTenantInfo -> TenantInfo

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, @herkolategan, @michae2, @smg260, and @stevendanna)

pkg/sql/parser/sql.y line 16103 at r15 (raw file):

Previously, stevendanna (Steven Danna) wrote…

I've recently learned that we should potentially also add these new keywords to the bare_keywords list. But, I plan to do a pass for all the keywords added in 23.1 soon.

Thanks I'll let you do this then.

pkg/ccl/backupccl/testdata/backup-restore/restore-tenants line 18 at r15 (raw file):

Previously, stevendanna (Steven Danna) wrote…

We could update this test file to show that servce_state is properly restored during a tenant restore.

Let's do it! Done.

knz · 2023-01-25T22:56:05Z

This PR will conflict with #95013.
This latter PR is scheduled to merge tonight. I will give it a chance, then rebase mine tomorrow morning before merging.

The previous change had a few shortcomings: - did not properly separate prepare/planning from execute. - put evaluation code in the 'tree' package (we want it in sql) - didn't set a foundation for non-bool capabilities This commit fixes that. Release note: None

knz · 2023-01-26T09:27:00Z

bors r=stevendanna,dt,ajwerner

craig · 2023-01-26T10:08:54Z

Build failed:

Bazel Essential CI (Cockroach)

This commit contains the following changes: - rename "state" to "data_state", "active" to "ready" - stored, non-virtual columns for "name", "data_state", "service_mode" - deprecate the column "active" since it mirrors "data_state' - move `descpb.TenantInfo` to new package `mtinfopb`. - new statements `ALTER TENANT ... START SERVICE EXTERNAL/SHARED`, `STOP SERVICE` to change the service mode. Details follow. **rename TenantInfo.State to DataState, "ACTIVE" to "READY"** We've discovered that we'd like to separate the readiness of the data from the activation of the service. To emphasize this, this commit renames the field "State" to "DataState". Additionally, the state "ACTIVE" was confusing as it suggests that something is running, whereas it merely marks the tenant data as ready for use. So this commit also renames that state accordingly. **new tenant info field ServiceMode** Summary of changes: - the new TenantInfo.ServiceMode field indicates how to run servers. - new syntax: `ALTER TENANT ... START SERVICE EXTERNAL/SHARED`, `ALTER TENANT ... STOP SERVICE`. - tenants created via `create_tenant(<id>)` (via CC serverless control plane) start in service mode EXTERNAL. - other tenants start in service mode NONE. - need ALTER TENANT STOP SERVICE before dropping a tenant. - except in the case of `crdb_internal.destroy_tenant` for compat with CC serverless control plane. **make the output columns of SHOW TENANT lowercase** All the SHOW statements report status-like data in lowercase. SHOW TENANT(s) should not be different. **use actual SQL columns for the TenantInfo fields** Release note: None

knz · 2023-01-26T13:57:56Z

bors r=stevendanna,dt,ajwerner

craig · 2023-01-26T15:39:42Z

Build succeeded:

Bazel Essential CI (Cockroach)

95658: server: only run tenants with service mode 'shared' r=stevendanna a=knz Fixes #92739. Previous PRs: - [x] sql: improve tenant records #95691 Prior to this patch, services for secondary tenants would be started automatically upon first use by a client. This commit changes this to auto-start services upfront for all tenants with service mode SHARED. (And shut down services for tenants with another service mode configured.) Release note: None Epic: CRDB-21836 Co-authored-by: Raphael 'kena' Poss <[email protected]>

We aimed in cockroachdb#95691 to preserve compatibility with the previous semantics of `destroy_tenant()`, but we failed to do so and there was no test to tell us about the mistake. This commit fixes both. Release note: None

98209: sql: fix `destroy_tenant` r=stevendanna a=knz Fixes #97873. We aimed in #95691 to preserve compatibility with the previous semantics of `destroy_tenant()`, but we failed to do so and there was no test to tell us about the mistake. This commit fixes both. Release note: None Co-authored-by: Raphael 'kena' Poss <[email protected]>

knz force-pushed the 20230123-better-tenant-state branch from 35b2a26 to 41f5a04 Compare January 23, 2023 21:13

knz requested review from stevendanna, ajwerner and dt January 23, 2023 21:14

knz marked this pull request as ready for review January 23, 2023 21:14

knz requested a review from a team January 23, 2023 21:14

knz requested review from a team as code owners January 23, 2023 21:14

knz requested a review from a team January 23, 2023 21:14

knz requested a review from a team as a code owner January 23, 2023 21:14

knz requested a review from michae2 January 23, 2023 21:14

knz added the A-multitenancy Related to multi-tenancy label Jan 23, 2023

This was referenced Jan 23, 2023

sql: make the output columns of SHOW TENANT lowercase #95655

Closed

sql: rename TenantInfo.State to DataState, "ACTIVE" to "READY" #95574

Closed

sql,ccl: new tenant info field ServiceMode #95581

Closed

knz force-pushed the 20230123-better-tenant-state branch 2 times, most recently from d0e6bfa to 9006812 Compare January 23, 2023 21:58

knz requested review from a team as code owners January 23, 2023 21:58

knz requested review from herkolategan and smg260 and removed request for a team January 23, 2023 21:59

knz force-pushed the 20230123-better-tenant-state branch 2 times, most recently from c9bf3f9 to 0355340 Compare January 23, 2023 22:13

knz commented Jan 24, 2023

View reviewed changes

knz force-pushed the 20230123-better-tenant-state branch from 91ccefe to 9ffc6a8 Compare January 24, 2023 16:14

stevendanna reviewed Jan 24, 2023

View reviewed changes

knz force-pushed the 20230123-better-tenant-state branch from 9ffc6a8 to 61f2cd9 Compare January 25, 2023 21:14

knz commented Jan 25, 2023

View reviewed changes

knz requested a review from stevendanna January 25, 2023 21:14

stevendanna reviewed Jan 25, 2023

View reviewed changes

stevendanna approved these changes Jan 25, 2023

View reviewed changes

knz force-pushed the 20230123-better-tenant-state branch from 61f2cd9 to 1a0014b Compare January 25, 2023 22:30

knz commented Jan 25, 2023

View reviewed changes

knz force-pushed the 20230123-better-tenant-state branch from 1a0014b to 52787d8 Compare January 25, 2023 22:51

knz force-pushed the 20230123-better-tenant-state branch from 52787d8 to 7eda3b3 Compare January 26, 2023 09:13

stevendanna mentioned this pull request Jan 26, 2023

sql: add new keywords to bare_label_keyword list #95985

Closed

knz force-pushed the 20230123-better-tenant-state branch from 7eda3b3 to 5fcce46 Compare January 26, 2023 13:57

craig bot merged commit 6819d87 into cockroachdb:master Jan 26, 2023

jaylim-crl mentioned this pull request Mar 1, 2023

sql: crdb_internal.destroy_tenant no longer destroys active tenants #97873

Closed

knz mentioned this pull request Mar 8, 2023

sql: fix destroy_tenant #98209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: improve tenant records #95691

sql: improve tenant records #95691

knz commented Jan 23, 2023 •

edited

Loading

cockroach-teamcity commented Jan 23, 2023

knz commented Jan 23, 2023

knz left a comment

stevendanna left a comment

stevendanna Jan 24, 2023

knz left a comment •

edited

Loading

stevendanna left a comment

stevendanna Jan 25, 2023

stevendanna Jan 25, 2023

knz left a comment

knz commented Jan 25, 2023

knz commented Jan 26, 2023

craig bot commented Jan 26, 2023

knz commented Jan 26, 2023

craig bot commented Jan 26, 2023

		// DataStateReady indicates data is ready and SQL servers can access it.
		DataStateReady TenantDataState = 0

sql: improve tenant records #95691

sql: improve tenant records #95691

Conversation

knz commented Jan 23, 2023 • edited Loading

cockroach-teamcity commented Jan 23, 2023

knz commented Jan 23, 2023

knz left a comment

Choose a reason for hiding this comment

stevendanna left a comment

Choose a reason for hiding this comment

stevendanna Jan 24, 2023

Choose a reason for hiding this comment

knz left a comment • edited Loading

Choose a reason for hiding this comment

stevendanna left a comment

Choose a reason for hiding this comment

stevendanna Jan 25, 2023

Choose a reason for hiding this comment

stevendanna Jan 25, 2023

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

knz commented Jan 25, 2023

knz commented Jan 26, 2023

craig bot commented Jan 26, 2023

knz commented Jan 26, 2023

craig bot commented Jan 26, 2023

knz commented Jan 23, 2023 •

edited

Loading

knz left a comment •

edited

Loading