sql: add ability set, edit, read tenant capabilities #95013

ecwall · 2023-01-10T19:33:49Z

Fixes #87851

Add new SQL syntax for

Setting tenant capabilities:
ALTER TENANT t GRANT CAPABILITY capabilitiy_name=capability_value;
Resetting tenant capabilities:
ALTER TENANT t REVOKE CAPABILITIY capability_name;
Reading tenant capabilities:
SHOW TENANT t WITH CAPABILITIES;

Release note: None

cockroach-teamcity · 2023-01-10T19:33:58Z

This change is

ecwall · 2023-01-10T19:51:41Z

This should be rebased and updated to use tenant_spec after #94932 is merged.

dikshant · 2023-01-12T20:37:17Z

Is 3) supposed to be:

SHOW TENANT t WITH CAPABILITY capability_name;

or is SHOW TENANT t WITH CAPABILITY; supposed to list all capabilities for a tenant?

knz

General direction OK.
Regarding our convo yesterday: I like the idea to have the base statement be SHOW TENANT, and WITH modify its behavior to add more columns. See my later comments about that.

Further than that:

you forgot to add/update sql/tree/walk.go to properly walk the exprs in AlterTenantSetCapabilities
As to dikshant's question, if the thing lists all capabilities it should be WITH CAPABILITIES (plural), perhaps with the singular as a non-documented convenience alias.
GRANT/REVOKE, not SET/RESET
we want the ability to grant/revoke multiple capabilities at once, with a comma-separated list.

Reviewed 17 of 17 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @dikshant, @ecwall, and @rafiss)

pkg/sql/parser/sql.y line 5557 at r1 (raw file):

   }
  }
| SHOW TENANT d_expr WITH CAPABILITY

I would combine them with a show_tenant_with_clause, accepting a list of modifiers (like SHOW RANGES does)
Then the execution code would add columns as specified by the modifiers, instead of changing the result schema entirely.

pkg/sql/sem/tree/stmt.go line 452 at r1 (raw file):

func (*AlterTenantCapability) StatementTag() string { return "ALTER TENANT CAPABILITY" }

func (*AlterTenantCapability) cclOnlyStatement() {}

Probably not.

ecwall · 2023-01-19T14:06:38Z

Is 3) supposed to be:
SHOW TENANT t WITH CAPABILITY capability_name;
or is SHOW TENANT t WITH CAPABILITY; supposed to list all capabilities for a tenant?

I'm changing it to WITH CAPABILITIES; and it will show all of them.

You can filter the results if you want only a specific one to appear:
SELECT * FROM [SHOW TENANT t WITH CAPABILITIES] WHERE ...

ecwall

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @dikshant, @knz, and @rafiss)

pkg/sql/sem/tree/stmt.go line 452 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

Probably not.

Good catch I just copied these methods.

ecwall

Added
Changed to WITH CAPABILITIES for now. I don't know if we should have an alias for this but not the other ALTER TENANT statements for plural/singular.
Changed
Added

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @dikshant, @knz, and @rafiss)

pkg/sql/parser/sql.y line 5557 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

I would combine them with a show_tenant_with_clause, accepting a list of modifiers (like SHOW RANGES does)
Then the execution code would add columns as specified by the modifiers, instead of changing the result schema entirely.

Updated.

knz

Reviewed 5 of 16 files at r2, 20 of 20 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @dikshant, @ecwall, and @rafiss)

pkg/sql/parser/sql.y line 5592 at r3 (raw file):

// %Category: Experimental
// %Text:
// SHOW { TENANT { <tenant_spec> | ALL } | TENANTS ] [WITH {REPLICATION STATUS|CAPABILITY}]

nit: also mention SHOW TENANTS on a separate line.

pkg/sql/parser/sql.y line 5624 at r3 (raw file):

  { $$.val = $2.showTenantOpts() }

show_tenant_options:

nit:

show_tenant_options:
   show_tenant_option
| show_tenant_options ',' show_tenant_option { $$.val = append($1.showTenantOpts(), $3.showTenantOpt()) }

show_tenant_option:
   REPLICATION STATUS
| CAPABILITIES

pkg/sql/parser/sql.y line 6342 at r3 (raw file):

      TenantSpec: $3.tenantSpec(),
      Capabilities: $6.tenantCapabilities(),
			IsReset: true,

nit: spaces not tabs

pkg/sql/parser/sql.y line 6347 at r3 (raw file):

grant_tenant_capability:
  var_name to_or_eq var_value

The = <value> part is optional. Some capabilities are just an identifier without a value.

pkg/sql/parser/sql.y line 6355 at r3 (raw file):

  }

grant_tenant_capability_list:

make a single non-terminal tenant_capability_list for both grant and revoke in the grammar (both accept = var_value)
during planning, produce an error in REVOKE if there are assignments.

ecwall

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @dikshant, @knz, and @rafiss)

pkg/sql/parser/sql.y line 5592 at r3 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

nit: also mention SHOW TENANTS on a separate line.

You mean this?

SHOW TENANT { <tenant_spec> | ALL } [ WITH { REPLICATION STATUS|CAPABILITY } ]
SHOW TENANTS [ WITH { REPLICATION STATUS|CAPABILITY } ]

pkg/sql/parser/sql.y line 5624 at r3 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

nit:

show_tenant_options:
   show_tenant_option
| show_tenant_options ',' show_tenant_option { $$.val = append($1.showTenantOpts(), $3.showTenantOpt()) }

show_tenant_option:
   REPLICATION STATUS
| CAPABILITIES

I modeled this after show_ranges_options. It looks like it is this way in order to handle either being put first?

They aren't actually a slice of options, they modify the same "options" struct.

pkg/sql/parser/sql.y line 6342 at r3 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

nit: spaces not tabs

Fixed.

Code quote:

IsReset: true,

pkg/sql/parser/sql.y line 6347 at r3 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

The = <value> part is optional. Some capabilities are just an identifier without a value.

Boolean cluster settings get set to true instead of just being set with no value.

Why handle boolean capabilities differently?

knz

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @dikshant, @ecwall, and @rafiss)

pkg/sql/parser/sql.y line 5592 at r3 (raw file):

Previously, ecwall (Evan Wall) wrote…

You mean this?

SHOW TENANT { <tenant_spec> | ALL } [ WITH { REPLICATION STATUS|CAPABILITY } ]
SHOW TENANTS [ WITH { REPLICATION STATUS|CAPABILITY } ]

yes. You can also do this:

// SHOW TENANT { <tenant_spec> | ALL }  [ WITH <options> ]
// SHOW TENANTS                         [ WITH <options> ]
//
// Options:
//     REPLICATION STATUS
//     CAPABILITIES

(nb: CAPABILITIES, not CAPABILITY)

pkg/sql/parser/sql.y line 5624 at r3 (raw file):

Previously, ecwall (Evan Wall) wrote…

I modeled this after show_ranges_options. It looks like it is this way in order to handle either being put first?

They aren't actually a slice of options, they modify the same "options" struct.

Oh, that's right. Then what you did is ok.

pkg/sql/parser/sql.y line 6347 at r3 (raw file):

Previously, ecwall (Evan Wall) wrote…

Boolean cluster settings get set to true instead of just being set with no value.

Why handle boolean capabilities differently?

We have used this pattern elsewhere already. For example, BACKUP ... WITH detached is implicit for WITH detached = true.

The advantage of supporting this is that you can then use a single grammar rule for both GRANT and REVOKE.

ecwall

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @dikshant, @knz, and @rafiss)

pkg/sql/parser/sql.y line 5592 at r3 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

yes. You can also do this:

// SHOW TENANT { <tenant_spec> | ALL }  [ WITH <options> ]
// SHOW TENANTS                         [ WITH <options> ]
//
// Options:
//     REPLICATION STATUS
//     CAPABILITIES

(nb: CAPABILITIES, not CAPABILITY)

Updated.

pkg/sql/parser/sql.y line 6347 at r3 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

We have used this pattern elsewhere already. For example, BACKUP ... WITH detached is implicit for WITH detached = true.

The advantage of supporting this is that you can then use a single grammar rule for both GRANT and REVOKE.

Ok, updated.

pkg/sql/parser/sql.y line 6355 at r3 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

make a single non-terminal tenant_capability_list for both grant and revoke in the grammar (both accept = var_value)

during planning, produce an error in REVOKE if there are assignments.

Updated based on comment above.

knz

with nit

Reviewed 17 of 18 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani, @dikshant, @ecwall, and @rafiss)

pkg/sql/tenant_capability.go line 64 at r5 (raw file):

			capabilities.CanAdminSplit, err = capability.GetBoolValue(isRevoke)
		default:
			err = errors.Newf("invalid capability")

"invalid capability: %q", capabilityName

Fixes #87851 Add new SQL syntax for 1) Setting tenant capabilities: `ALTER TENANT t GRANT CAPABILITY capabilitiy_name=capability_value;` 2) Resetting tenant capabilities: `ALTER TENANT t REVOKE CAPABILITIY capability_name;` 3) Reading tenant capabilities: `SHOW TENANT t WITH CAPABILITIES;` Release note: None

ecwall

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @arulajmani, @dikshant, @knz, and @rafiss)

pkg/sql/tenant_capability.go line 64 at r5 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

"invalid capability: %q", capabilityName

I handled adding the capabilityName for this error and the errors from GetBoolValue (and future Get*Value methods) below:

		if err != nil {
			return pgerror.Wrapf(
				err,
				pgcode.InvalidParameterValue,
				"error parsing capability %s",  // I will change this to %q though based on your feedback
				capabilityName,
			)
		}

so they would have a more consistent format:

statement error error parsing capability not_a_capability: invalid capability
ALTER TENANT [5] GRANT CAPABILITY not_a_capability=true

statement error error parsing capability can_admin_split: value must be bool
ALTER TENANT [5] GRANT CAPABILITY can_admin_split=1

statement error error parsing capability not_a_capability: invalid capability
ALTER TENANT [5] REVOKE CAPABILITY not_a_capability

statement error error parsing capability can_admin_split: revoke must not specify value
ALTER TENANT [5] REVOKE CAPABILITY can_admin_split=false

ecwall · 2023-01-25T22:36:54Z

bors r=knz

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @arulajmani, @dikshant, @ecwall, and @rafiss)

pkg/sql/tenant_capability.go line 64 at r5 (raw file):

Previously, ecwall (Evan Wall) wrote…

I handled adding the capabilityName for this error and the errors from GetBoolValue (and future Get*Value methods) below:

		if err != nil {
			return pgerror.Wrapf(
				err,
				pgcode.InvalidParameterValue,
				"error parsing capability %s",  // I will change this to %q though based on your feedback
				capabilityName,
			)
		}

so they would have a more consistent format:

statement error error parsing capability not_a_capability: invalid capability
ALTER TENANT [5] GRANT CAPABILITY not_a_capability=true

statement error error parsing capability can_admin_split: value must be bool
ALTER TENANT [5] GRANT CAPABILITY can_admin_split=1

statement error error parsing capability not_a_capability: invalid capability
ALTER TENANT [5] REVOKE CAPABILITY not_a_capability

statement error error parsing capability can_admin_split: revoke must not specify value
ALTER TENANT [5] REVOKE CAPABILITY can_admin_split=false

You did not include the capability name in the error generated if the capability name is not recognized -- e.g. alter tenant .. revoke capability THIS_CAPABILITY_DOES_NOT_EXIST. We need to provide more detail in that error too.

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @arulajmani, @dikshant, @ecwall, and @rafiss)

pkg/sql/tenant_capability.go line 64 at r5 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

You did not include the capability name in the error generated if the capability name is not recognized -- e.g. alter tenant .. revoke capability THIS_CAPABILITY_DOES_NOT_EXIST. We need to provide more detail in that error too.

oh and that case deserves a test as well.

craig · 2023-01-26T00:01:48Z

Build succeeded:

Bazel Essential CI (Cockroach)

ecwall requested review from knz, rafiss, arulajmani and a team January 10, 2023 19:33

ecwall requested a review from a team as a code owner January 10, 2023 19:33

ecwall requested a review from dikshant January 12, 2023 17:30

knz reviewed Jan 13, 2023

View reviewed changes

ecwall commented Jan 20, 2023

View reviewed changes

ecwall requested a review from a team as a code owner January 20, 2023 20:43

ecwall requested a review from knz January 20, 2023 20:44

knz reviewed Jan 23, 2023

View reviewed changes

ecwall commented Jan 23, 2023

View reviewed changes

ecwall requested a review from knz January 24, 2023 13:52

knz reviewed Jan 25, 2023

View reviewed changes

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

ecwall commented Jan 25, 2023

View reviewed changes

knz approved these changes Jan 25, 2023

View reviewed changes

ecwall commented Jan 25, 2023

View reviewed changes

knz reviewed Jan 25, 2023

View reviewed changes

knz mentioned this pull request Jan 25, 2023

sql: improve tenant records #95691

Merged

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

irfansharif mentioned this pull request Mar 9, 2023

sql: support manual splits on table boundaries for secondary tenants #65903

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add ability set, edit, read tenant capabilities #95013

sql: add ability set, edit, read tenant capabilities #95013

ecwall commented Jan 10, 2023 •

edited

Loading

cockroach-teamcity commented Jan 10, 2023

ecwall commented Jan 10, 2023

dikshant commented Jan 12, 2023 •

edited

Loading

knz left a comment

ecwall commented Jan 19, 2023

ecwall left a comment

ecwall left a comment

knz left a comment

ecwall left a comment

knz left a comment

ecwall left a comment

knz left a comment

ecwall left a comment

ecwall commented Jan 25, 2023

knz left a comment

knz left a comment

craig bot commented Jan 26, 2023

sql: add ability set, edit, read tenant capabilities #95013

sql: add ability set, edit, read tenant capabilities #95013

Conversation

ecwall commented Jan 10, 2023 • edited Loading

cockroach-teamcity commented Jan 10, 2023

ecwall commented Jan 10, 2023

dikshant commented Jan 12, 2023 • edited Loading

knz left a comment

Choose a reason for hiding this comment

ecwall commented Jan 19, 2023

ecwall left a comment

Choose a reason for hiding this comment

ecwall left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

ecwall left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

ecwall left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

ecwall left a comment

Choose a reason for hiding this comment

ecwall commented Jan 25, 2023

knz left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

craig bot commented Jan 26, 2023

ecwall commented Jan 10, 2023 •

edited

Loading

dikshant commented Jan 12, 2023 •

edited

Loading