sql: add a SHOW TENANTS command #94734

lidorcarmel · 2023-01-05T01:19:04Z

This command prints all available tenants and their status.
To get the status for replication (c2c) tenants we need to get the job info.

For example:

    [email protected]:26257/defaultdb> show tenants;
      id |   name   |                               status
    -----+----------+----------------------------------------------------------------------
       1 | system   | ACTIVE
       2 | src      | ACTIVE
       3 | dest     | REPLICATION PAUSED
       4 | dest2    | REPLICATION UNKNOWN (succeeded)
       6 | dest3    | REPLICATION UNKNOWN (job with ID 819888096954253313 does not exist)
      11 | dest4    | ACTIVE
      12 | dest5    | REPLICATING
      13 | dest6    | INITIALIZING REPLICATION
    (13 rows)

    Time: 67ms total (execution 67ms / network 0ms)

Fixes: #92641

Epic: CRDB-18749

Release note: None

cockroach-teamcity · 2023-01-05T01:19:20Z

This change is

adityamaru · 2023-01-09T20:00:23Z

pkg/ccl/streamingccl/streamingest/alter_replication_job.go

@@ -161,7 +161,7 @@ func alterReplicationJobHook(
 			return err
 		}
 		if tenInfo.TenantReplicationJobID == 0 {
-			return errors.Newf("tenant %q does not have an active replication job", tenantName)
+			return errors.Newf("tenant %q is not a replicated tenant", tenantName)


hmm is this change necessary? Tenant foo could have been created as a result of c2c replication, so it could be confusing to say that it is not a replicated tenant if the job ID is 0. IMO it is sufficient to say that there is no active replication job associated with this tenant and so the replication information you are trying to access is unavailable.

not necessary in this pr, dropping.
FWIW I think we should avoid saying "job" when possible.. ( I executed SHOW TENANT WITH REPLICATION STATUS why are you mentioning jobs?).

adityamaru · 2023-01-09T20:07:59Z

pkg/sql/tenant.go

+
+	tenants := make([]roachpb.TenantName, 0, len(rows))
+	for _, tenant := range rows {
+		// If the tenants is in the DROP state the then name is nil.


I believe tenant name can also be zero if it was created prior to V23_1TenantNames, or using the create_tenant(id) builtin.

I was assuming future tenants will always have a name, but maybe there is no need to assume that.. and also, it would be nice to show dropped tenants. anyway, changed this code to use ids instead of names.

adityamaru · 2023-01-09T20:08:49Z

pkg/sql/tenant.go

@@ -202,6 +202,33 @@ func CreateTenantRecord(
 	)
 }

+func GetAllTenantNames(


nit: linter might scream at you to add a comment to this exported method

it should! but it didn't.. fixed.

pkg/sql/show_tenant.go

adityamaru · 2023-01-09T20:48:40Z

pkg/sql/show_tenant.go

 	default:
-		return errors.Newf("unknown tenant status %s", n.tenantInfo.State)
+		status := fmt.Sprintf("tenant status: %s", values.tenantInfo.State)


I prefer if we leave this an error. If we see a tenant in a state other than ADD, DROP, ACTIVE then it means someone has added a state and not considered all the places it effects, so I'd rather it fail tests loudly. wdyt?

sure, done. I don't like it that the show command just breaks when one tenant has an issue but here it's fine.

pkg/sql/show_tenant.go

stevendanna · 2023-01-10T13:03:30Z

pkg/sql/show_tenant.go

+
+func (n *showTenantNode) getTenantValues(
+	params runParams, tenantName roachpb.TenantName,
+) (*tenantValues, error) {
 	tenantRecord, err := GetTenantRecordByName(params.ctx, params.p.execCfg, params.p.Txn(), tenantName)


As a heads up, there is another PR in flight to make most of these commands work with either IDs or names. Not sure there is anything we want to do here other than being willing to help resolve the conflict when it occurs :D.

got it, thanks.

stevendanna · 2023-01-10T13:18:19Z

pkg/sql/show_tenant.go

@@ -93,100 +110,145 @@ func (n *showTenantNode) getTenantName(params runParams) (roachpb.TenantName, er
 }

 func (n *showTenantNode) startExec(params runParams) error {
-	tenantName, err := n.getTenantName(params)
+	if n.all {
+		names, err := GetAllTenantNames(params.ctx, params.p.execCfg, params.p.Txn())


I'm wondering, instead of passing this list of names around. Could we do more of this in sql? That is, we are essentially pretty-printing the results of SELECT id, FROM system.tenants and annotating it with job details. I wonder if we could either: (1) use an iterator over a query we construct to select the correct nodes or (2) implement this as a delegated statement instead.

I'm OK doing this as a follow-up, just a little suspect of anywhere that we require passing around complete sets of user-generatable data.

good point, as we discussed we might want to do this in a followup.

lidorcarmel

unfortunately too many changes :(
PTAL
mostly - this pr now gets the list of ids, not names, which means we also show dropped tenants without a name! (I thought of putting the droppedName there but then the name would not be unique which is not fun). because of that this pr is sorting the list by tenant id (probably better than by name?).
added some tests.
thanks!!

lidorcarmel · 2023-01-09T22:53:42Z

pkg/ccl/streamingccl/streamingest/alter_replication_job.go

@@ -161,7 +161,7 @@ func alterReplicationJobHook(
 			return err
 		}
 		if tenInfo.TenantReplicationJobID == 0 {
-			return errors.Newf("tenant %q does not have an active replication job", tenantName)
+			return errors.Newf("tenant %q is not a replicated tenant", tenantName)


not necessary in this pr, dropping.
FWIW I think we should avoid saying "job" when possible.. ( I executed SHOW TENANT WITH REPLICATION STATUS why are you mentioning jobs?).

lidorcarmel · 2023-01-10T18:46:22Z

pkg/sql/show_tenant.go

+
+func (n *showTenantNode) getTenantValues(
+	params runParams, tenantName roachpb.TenantName,
+) (*tenantValues, error) {
 	tenantRecord, err := GetTenantRecordByName(params.ctx, params.p.execCfg, params.p.Txn(), tenantName)


got it, thanks.

pkg/sql/show_tenant.go

lidorcarmel · 2023-01-10T19:01:35Z

pkg/sql/show_tenant.go

 	default:
-		return errors.Newf("unknown tenant status %s", n.tenantInfo.State)
+		status := fmt.Sprintf("tenant status: %s", values.tenantInfo.State)


sure, done. I don't like it that the show command just breaks when one tenant has an issue but here it's fine.

lidorcarmel · 2023-01-10T19:04:19Z

pkg/sql/tenant.go

@@ -202,6 +202,33 @@ func CreateTenantRecord(
 	)
 }

+func GetAllTenantNames(


it should! but it didn't.. fixed.

lidorcarmel · 2023-01-10T23:42:40Z

pkg/sql/tenant.go

+
+	tenants := make([]roachpb.TenantName, 0, len(rows))
+	for _, tenant := range rows {
+		// If the tenants is in the DROP state the then name is nil.


I was assuming future tenants will always have a name, but maybe there is no need to assume that.. and also, it would be nice to show dropped tenants. anyway, changed this code to use ids instead of names.

lidorcarmel · 2023-01-10T23:45:05Z

pkg/sql/show_tenant.go

@@ -93,100 +110,145 @@ func (n *showTenantNode) getTenantName(params runParams) (roachpb.TenantName, er
 }

 func (n *showTenantNode) startExec(params runParams) error {
-	tenantName, err := n.getTenantName(params)
+	if n.all {
+		names, err := GetAllTenantNames(params.ctx, params.p.execCfg, params.p.Txn())


good point, as we discussed we might want to do this in a followup.

stevendanna

Overall, this seems reasonable to me. I think a few new logic tests would be great though.

stevendanna · 2023-01-12T10:39:42Z

pkg/sql/tenant.go

@@ -201,6 +201,35 @@ func CreateTenantRecord(
 	)
 }

+// GetAllTenantIds returns all tenants in the system table, excluding those in
+// the DROP state.
+func GetAllTenantIds(


[nit] Ids -> IDs. Should this be GetAllNonDropTenantIDs?

stevendanna · 2023-01-12T10:41:18Z

pkg/sql/tenant.go

+		iTenantId := uint64(tree.MustBeDInt(tenant[0]))
+		tenantId, err := roachpb.MakeTenantID(iTenantId)
+		if err != nil {
+			return nil, err


We might want to make this an AssertionFailedf since it implies we allowed someone to write an invalid tenant ID into the tenants table.

stevendanna · 2023-01-12T10:41:59Z

pkg/sql/logictest/testdata/logic_test/tenant

@@ -76,10 +76,10 @@ id  name   status
 statement error tenant "seven" does not exist
 SHOW TENANT seven

-statement error tenant "tenant-one" does not have an active replication job
+query error pq: tenant "tenant-one" does not have an active replication job


Could we add a test to this file to show the output of SHOW TENANTS?

definitely, forgot about those.. thanks! done. also added some in the tenant datadriven test.

lidorcarmel

thanks! done.

lidorcarmel · 2023-01-12T17:25:12Z

pkg/sql/tenant.go

@@ -201,6 +201,35 @@ func CreateTenantRecord(
 	)
 }

+// GetAllTenantIds returns all tenants in the system table, excluding those in
+// the DROP state.
+func GetAllTenantIds(


lidorcarmel · 2023-01-12T17:26:19Z

pkg/sql/logictest/testdata/logic_test/tenant

@@ -76,10 +76,10 @@ id  name   status
 statement error tenant "seven" does not exist
 SHOW TENANT seven

-statement error tenant "tenant-one" does not have an active replication job
+query error pq: tenant "tenant-one" does not have an active replication job


definitely, forgot about those.. thanks! done. also added some in the tenant datadriven test.

lidorcarmel · 2023-01-12T17:37:55Z

pkg/sql/tenant.go

+		iTenantId := uint64(tree.MustBeDInt(tenant[0]))
+		tenantId, err := roachpb.MakeTenantID(iTenantId)
+		if err != nil {
+			return nil, err


lidorcarmel requested review from stevendanna and adityamaru January 5, 2023 01:19

lidorcarmel requested a review from a team as a code owner January 5, 2023 01:19

lidorcarmel requested a review from a team January 5, 2023 01:19

lidorcarmel requested a review from a team as a code owner January 5, 2023 01:19

lidorcarmel requested review from smg260 and renatolabs and removed request for a team January 5, 2023 01:19

lidorcarmel force-pushed the lidor_show_tenants branch 6 times, most recently from f97766d to 62c6e2b Compare January 7, 2023 00:39

adityamaru suggested changes Jan 9, 2023

View reviewed changes

stevendanna reviewed Jan 10, 2023

View reviewed changes

adityamaru self-requested a review January 10, 2023 14:22

lidorcarmel force-pushed the lidor_show_tenants branch from 62c6e2b to bb968da Compare January 11, 2023 00:04

lidorcarmel commented Jan 11, 2023

View reviewed changes

lidorcarmel force-pushed the lidor_show_tenants branch 6 times, most recently from f983dfc to 72c5903 Compare January 12, 2023 01:27

stevendanna reviewed Jan 12, 2023

View reviewed changes

lidorcarmel force-pushed the lidor_show_tenants branch from 72c5903 to fc4bc05 Compare January 12, 2023 18:31

lidorcarmel commented Jan 12, 2023

View reviewed changes

adityamaru approved these changes Jan 12, 2023

View reviewed changes

lidorcarmel force-pushed the lidor_show_tenants branch from 93d45f7 to c6acd52 Compare January 12, 2023 19:34