settings: Add syntax for cluster settings by ajstorm · Pull Request #76929 · cockroachdb/cockroach

ajstorm · 2022-02-23T16:05:39Z

Before this commit, there was no syntax to SET or SHOW cluster settings which
exist for a given tenant. This commit adds the following syntax:

ALTER TENANT SET CLUSTER SETTING =
ALTER TENANT ALL SET CLUSTER SETTING =
ALTER TENANT RESET CLUSTER SETTING
ALTER TENANT ALL RESET CLUSTER SETTING
SHOW CLUSTER SETTING FOR TENANT
SHOW [ALL] CLUSTER SETTINGS FOR TENANT

Note that the syntax is added but the underlying commands are currently
unimplemented. The implementation of these commands will come with a subsequent
commit.

Release note (sql change): Added syntax for modifying cluster settings at the
tenant level.

Informs: #73857.

cockroach-teamcity · 2022-02-23T16:05:46Z

This change is

rafiss

just had nits and needs tests!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @ajwerner, @dt, and @RaduBerinde)

pkg/sql/set_cluster_setting.go, line 178 at r1 (raw file):

	if n.tenantID.IsSet() || n.tenantAll {
		return errors.UnimplementedError(

nit: use unimplemented.NewWithIssue

pkg/sql/show_cluster_setting.go, line 122 at r1 (raw file):

) (planNode, error) {
	if n.TenantID.IsSet() {
		return nil, errors.UnimplementedError(

nit: use unimplemented.NewWithIssue

pkg/sql/delegate/show_all_cluster_settings.go, line 26 at r1 (raw file):

) (tree.Statement, error) {
	if stmt.TenantID.IsSet() {
		return nil, errors.UnimplementedError(

nit: use unimplemented.NewWithIssue

pkg/sql/sem/tree/set.go, line 81 at r1 (raw file):

// Format implements the NodeFormatter interface.
func (node *SetClusterSetting) Format(ctx *FmtCtx) {

we should test this formatting logic as well. could you add test cases to pkg/sql/parser/testdata/show and pkg/sql/parser/testdata/set (or a new test file if you like)

ajstorm

Thanks Rafi.

I've addressed your comments, but you might want to have another full look. I reworked things considerably due to @RaduBerinde's suggestion that we create a new tree node for alter tenant.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, @RaduBerinde, and @rafiss)

pkg/sql/sem/tree/set.go, line 81 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

we should test this formatting logic as well. could you add test cases to pkg/sql/parser/testdata/show and pkg/sql/parser/testdata/set (or a new test file if you like)

Done.

rafiss

looks good overall! just one comment on the formatting

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @ajwerner, @dt, @RaduBerinde, and @rafiss)

pkg/sql/sem/tree/alter_tenant.go, line 33 at r2 (raw file):

		ctx.WriteString("ALTER TENANT ALL ")
	} else if node.TenantID.IsSet() {
		s := fmt.Sprintf("ALTER TENANT %d ", node.TenantID.ToUint64())

i don't think we should make the TenantID skip the FmtCtx flags (unless there's a specific reason?)

so we'd want:

ctx.WriteString("ALTER TENANT ")
ctx.FormatNode(node.TenantID)
ctx.WriteString(" ")

(same goes for SHOW formatting)

pkg/sql/parser/testdata/alter_tenant, line 6 at r2 (raw file):

ALTER TENANT 1 SET CLUSTER SETTING a = 3
ALTER TENANT 1 SET CLUSTER SETTING a = (3) -- fully parenthesized
ALTER TENANT 1 SET CLUSTER SETTING a = _ -- literals removed

i would expect the 1 to be removed in the "literals removed" step.

ajstorm

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @ajwerner, @dt, @RaduBerinde, and @rafiss)

pkg/sql/sem/tree/alter_tenant.go, line 33 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i don't think we should make the TenantID skip the FmtCtx flags (unless there's a specific reason?)

so we'd want:
ctx.WriteString("ALTER TENANT ")
ctx.FormatNode(node.TenantID)
ctx.WriteString(" ")
(same goes for SHOW formatting)

I was thinking that the tenant ID would never contain PII, but could be useful in debugging in all cases? Is this the proper justification for having it skip the flags?

rafiss · 2022-02-28T16:59:01Z

I was thinking that the tenant ID would never contain PII, but could be useful in debugging in all cases? Is this the proper justification for having it skip the flags?

It's a good intuition, but our formatting logic has two separate notions:

	// FmtHideConstants instructs the pretty-printer to produce a
	// representation that does not disclose query-specific data. It
	// also shorten long lists in tuples, VALUES and array expressions.
	FmtHideConstants

	// FmtAnonymize instructs the pretty-printer to remove
	// any name but function names.
	FmtAnonymize

FmtHideConstants only hides literal constants, but not identifiers. It is used, for example, (1) in the DB console statements page, to group statements together to be able to view query stats and (2) by the optimizer to cache query plans. (Neither (1) nor (2) seem that important for ALTER TENANT ... SET, but it's worth using the same formatting principles across our entire SQL syntax.)

FmtAnonymize is used to hide PII like names and identifiers, but not literal constants. For logging and telemetry, I believe FmtAnonymize and FmtHideConstants are both used.

ajstorm

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, @RaduBerinde, and @rafiss)

pkg/sql/sem/tree/alter_tenant.go, line 33 at r2 (raw file):

Previously, ajstorm (Adam Storm) wrote…

I was thinking that the tenant ID would never contain PII, but could be useful in debugging in all cases? Is this the proper justification for having it skip the flags?

As mentioned on Slack, I'm going to leave this as-is for now, pending the broader investigation as to what to do here.

pkg/sql/parser/testdata/alter_tenant, line 6 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i would expect the 1 to be removed in the "literals removed" step.

Does this key off of the Format function? If so I think we're stuck with showing this literal pending the investigation mentioned above.

ajstorm

I'm going to hold off on that change to the TenantID formatting. This should be RFAL now.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, @RaduBerinde, and @rafiss)

rafiss

lgtm!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @ajwerner, @dt, @RaduBerinde, and @rafiss)

pkg/sql/parser/testdata/alter_tenant, line 6 at r2 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Does this key off of the Format function? If so I think we're stuck with showing this literal pending the investigation mentioned above.

yeah, this is the result of formatting with FmtHideConstants. we can address it during the stability period

RaduBerinde

modulo one comment

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajstorm, @ajwerner, @dt, and @rafiss)

pkg/sql/alter_tenant.go, line 60 at r3 (raw file):

	}

	if setting.Class() == settings.SystemOnly && !p.execCfg.Codec.ForSystemTenant() {

Here we need to error out if it is a SystemOnly setting. For all others, we need to check ForSystemTenant() and only allow that (I think that should be the first thing to check - this is a "system/operator only" statement).

ajstorm

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajstorm, @ajwerner, @dt, @RaduBerinde, and @rafiss)

pkg/sql/alter_tenant.go, line 60 at r3 (raw file):

Previously, RaduBerinde wrote…

Here we need to error out if it is a SystemOnly setting. For all others, we need to check ForSystemTenant() and only allow that (I think that should be the first thing to check - this is a "system/operator only" statement).

@RaduBerinde I don't think I fully appreciated how we should error out here from a first read of the RFC. I took another crack at this logic based on your comments and I think I've got it right now. PTAL (assuming you're still checking email) and let me know what you think.

RaduBerinde

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajstorm, @ajwerner, @dt, @RaduBerinde, and @rafiss)

pkg/sql/alter_tenant.go, line 67 at r5 (raw file):

			"%s is a system-only setting and must be set using SET CLUSTER SETTING", name)
	}
	if !p.execCfg.Codec.ForSystemTenant() && setting.Class() != settings.TenantWritable {

This should just be `if !ForSystemTenant() and I'd move it to be the first check.

ALTER TENANT should only be runnable by the system tenant. Non-system tenants can only use SET CLUSTER SETTING to change their settings. ALTER TENANT is only about operator-set overrides. It's very similar to calling crdb_internal.create_tenant() from the standpoint of who can run it.

ajstorm

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajstorm, @ajwerner, @dt, @RaduBerinde, and @rafiss)

pkg/sql/alter_tenant.go, line 67 at r5 (raw file):

Previously, RaduBerinde wrote…

This should just be `if !ForSystemTenant() and I'd move it to be the first check.

ALTER TENANT should only be runnable by the system tenant. Non-system tenants can only use SET CLUSTER SETTING to change their settings. ALTER TENANT is only about operator-set overrides. It's very similar to calling crdb_internal.create_tenant() from the standpoint of who can run it.

Done.

Before this commit, there was no syntax to SET or SHOW cluster settings which exist for a given tenant. This commit adds the following syntax: * ALTER TENANT <id> SET CLUSTER SETTING <setting> = <value> * ALTER TENANT ALL SET CLUSTER SETTING <setting> = <value> * ALTER TENANT <id> RESET CLUSTER SETTING <setting> * ALTER TENANT ALL RESET CLUSTER SETTING <setting> * SHOW CLUSTER SETTING <setting> FOR TENANT <id> * SHOW [ALL] CLUSTER SETTINGS FOR TENANT <id> Note that the syntax is added but the underlying commands are currently unimplemented. The implementation of these commands will come with a subsequent commit. Release note (sql change): Added syntax for modifying cluster settings at the tenant level. Release justification: Completion of feature for 22.1.

ajstorm · 2022-03-02T11:49:52Z

TFTR!

bors r=raduberinde,rafiss

craig · 2022-03-02T14:25:29Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-03-02T16:49:11Z

Build succeeded:

GitHub CI (Cockroach)

RaduBerinde

Reviewable status: complete! 1 of 0 LGTMs obtained

ajstorm requested review from RaduBerinde and rafiss February 23, 2022 16:05

RaduBerinde requested review from ajwerner and dt February 23, 2022 16:29

rafiss requested changes Feb 23, 2022

View reviewed changes

ajstorm force-pushed the ajstorm-cs-syntax branch from 4fb5bc5 to d6ac859 Compare February 24, 2022 14:37

ajstorm requested a review from a team February 24, 2022 14:37

ajstorm force-pushed the ajstorm-cs-syntax branch 4 times, most recently from ac18423 to e2d0312 Compare February 24, 2022 15:55

ajstorm requested a review from rafiss February 24, 2022 18:22

ajstorm commented Feb 24, 2022

View reviewed changes

rafiss requested changes Feb 25, 2022

View reviewed changes

ajstorm requested a review from rafiss February 28, 2022 15:57

ajstorm commented Feb 28, 2022

View reviewed changes

ajstorm force-pushed the ajstorm-cs-syntax branch from e2d0312 to 5e6c2cf Compare February 28, 2022 17:52

ajstorm commented Feb 28, 2022

View reviewed changes

rafiss approved these changes Feb 28, 2022

View reviewed changes

RaduBerinde approved these changes Feb 28, 2022

View reviewed changes

ajstorm force-pushed the ajstorm-cs-syntax branch from 5e6c2cf to 867042b Compare February 28, 2022 22:15

ajstorm requested review from RaduBerinde and rafiss February 28, 2022 22:18

ajstorm commented Feb 28, 2022

View reviewed changes

ajstorm force-pushed the ajstorm-cs-syntax branch 3 times, most recently from c9983f7 to 3975247 Compare March 1, 2022 13:38

ajstorm force-pushed the ajstorm-cs-syntax branch from 3975247 to 30eb035 Compare March 1, 2022 14:22

ajstorm requested a review from a team as a code owner March 1, 2022 14:22

ajstorm force-pushed the ajstorm-cs-syntax branch 2 times, most recently from 5ddcb3b to 382c4da Compare March 1, 2022 19:35

RaduBerinde reviewed Mar 1, 2022

View reviewed changes

ajstorm force-pushed the ajstorm-cs-syntax branch 2 times, most recently from 638fe16 to c130708 Compare March 2, 2022 01:38

ajstorm requested a review from RaduBerinde March 2, 2022 01:39

ajstorm commented Mar 2, 2022

View reviewed changes

ajstorm force-pushed the ajstorm-cs-syntax branch from c130708 to 89091fe Compare March 2, 2022 01:43

craig bot merged commit 1cc1725 into cockroachdb:master Mar 2, 2022

cockroach-teamcity mentioned this pull request Mar 2, 2022

settings: Add syntax for cluster settings cockroachdb/docs#13149

Open

RaduBerinde reviewed Mar 3, 2022

View reviewed changes

knz mentioned this pull request Mar 11, 2022

settings: multi-tenant cluster settings tracking issue #73857

Closed

16 tasks

Conversation

ajstorm commented Feb 23, 2022

Uh oh!

cockroach-teamcity commented Feb 23, 2022

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

ajstorm left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

ajstorm left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss commented Feb 28, 2022

Uh oh!

ajstorm left a comment

Choose a reason for hiding this comment

Uh oh!

ajstorm left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

ajstorm left a comment

Choose a reason for hiding this comment

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

ajstorm left a comment

Choose a reason for hiding this comment

Uh oh!

ajstorm commented Mar 2, 2022

Uh oh!

craig bot commented Mar 2, 2022

Uh oh!

craig bot commented Mar 2, 2022

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants