sql: implement the logic for ALTER TENANT SET CLUSTER SETTING

docs/generated/eventlog.md

@@ -314,6 +314,33 @@ An event of type `set_cluster_setting` is recorded when a cluster setting is cha
 | `Value` | The new value of the cluster setting. | yes |
 
 
+#### Common fields
+
+| Field | Description | Sensitive |
+|--|--|--|
+| `Timestamp` | The timestamp of the event. Expressed as nanoseconds since the Unix epoch. | no |
+| `EventType` | The type of the event. | no |
+| `Statement` | A normalized copy of the SQL statement that triggered the event. The statement string contains a mix of sensitive and non-sensitive details (it is redactable). | partially |
+| `Tag` | The statement tag. This is separate from the statement string, since the statement string can contain sensitive information. The tag is guaranteed not to. | no |
+| `User` | The user account that triggered the event. The special usernames `root` and `node` are not considered sensitive. | depends |
+| `DescriptorID` | The primary object descriptor affected by the operation. Set to zero for operations that don't affect descriptors. | no |
+| `ApplicationName` | The application name for the session where the event was emitted. This is included in the event to ease filtering of logging output by application. Application names starting with a dollar sign (`$`) are not considered sensitive. | depends |
+| `PlaceholderValues` | The mapping of SQL placeholders to their values, for prepared statements. | yes |
+
+### `set_tenant_cluster_setting`
+
+An event of type `set_tenant_cluster_setting` is recorded when a cluster setting override
+is changed, either for another tenant or for all tenants.
+
+
+| Field | Description | Sensitive |
+|--|--|--|
+| `SettingName` | The name of the affected cluster setting. | no |
+| `Value` | The new value of the cluster setting. | yes |
+| `TenantId` | The target Tenant ID. Empty if targeting all tenants. | no |
+| `AllTenants` | Whether the override applies to all tenants. | no |
+
+
 #### Common fields
 
 | Field | Description | Sensitive |

pkg/ccl/logictestccl/testdata/logic_test/tenant_settings

@@ -7,9 +7,8 @@ true
 
 user host-cluster-root
 
-# TODO(radu): use the new ALTER TENANT statement when it is implemented.
 statement ok
-INSERT INTO system.tenant_settings (tenant_id, name, value, value_type) VALUES (10, 'sql.notices.enabled', 'false', 'b')
+ALTER TENANT 10 SET CLUSTER SETTING sql.notices.enabled = false
 
 user root
 
@@ -22,7 +21,7 @@ false
 user host-cluster-root
 
 statement ok
-DELETE FROM system.tenant_settings WHERE tenant_id = 10
+ALTER TENANT 10 RESET CLUSTER SETTING sql.notices.enabled
 
 user root
 
@@ -35,7 +34,7 @@ user host-cluster-root
 
 # Set an all-tenant override.
 statement ok
-INSERT INTO system.tenant_settings (tenant_id, name, value, value_type) VALUES (0, 'sql.notices.enabled', 'false', 'b')
+ALTER TENANT ALL SET CLUSTER SETTING sql.notices.enabled = false
 
 user root
 
@@ -48,7 +47,7 @@ user host-cluster-root
 
 # Now set a tenant-specific override which takes precedence.
 statement ok
-INSERT INTO system.tenant_settings (tenant_id, name, value, value_type) VALUES (10, 'sql.notices.enabled', 'true', 'b')
+ALTER TENANT 10 SET CLUSTER SETTING sql.notices.enabled = true
 
 user root
 
@@ -68,7 +67,7 @@ user host-cluster-root
 
 # Remove the all-tenant override; should make no difference.
 statement ok
-DELETE FROM system.tenant_settings WHERE tenant_id = 0
+ALTER TENANT ALL RESET CLUSTER SETTING sql.notices.enabled
 
 user root
 
@@ -99,8 +98,7 @@ SHOW CLUSTER SETTING kv.protectedts.reconciliation.interval
 user host-cluster-root
 
 statement ok
-INSERT INTO system.tenant_settings (tenant_id, name, value, value_type)
-  VALUES (10, 'kv.protectedts.reconciliation.interval', '45s', 'd')
+ALTER TENANT 10 SET CLUSTER SETTING kv.protectedts.reconciliation.interval = '45s'
 
 user root
 
@@ -115,9 +113,13 @@ user host-cluster-root
 statement ok
 SELECT crdb_internal.create_tenant(1234)
 
-# TODO(radu): replace with ALTER TENANT when it's available.
 statement ok
-INSERT INTO system.tenant_settings (tenant_id, name, value, value_type) VALUES (1234, 'sql.notices.enabled', 'true', 'b')
+ALTER TENANT 1234 SET CLUSTER SETTING sql.notices.enabled = true
+
+query I
+SELECT count(*) FROM system.tenant_settings WHERE tenant_id = 1234
+----
+1
 
 statement ok
 SELECT crdb_internal.destroy_tenant(1234, true)

pkg/sql/logictest/testdata/logic_test/cluster_settings

@@ -172,37 +172,53 @@ subtest tenant-cluster-settings
 # Skip this test when inside a tenant, as this setting won't be visible in
 # there.
 skipif config 3node-tenant
-statement error admission.kv.enabled is a system-only setting and must be set at the host cluster level using SET CLUSTER SETTING
+statement error admission.kv.enabled is a system-only setting and must be set in the admin tenant using SET CLUSTER SETTING
 ALTER TENANT 10 SET CLUSTER SETTING admission.kv.enabled=true
 
 onlyif config 3node-tenant
 statement error ALTER TENANT can only be called by system operators
 ALTER TENANT 10 SET CLUSTER SETTING server.mem_profile.total_dump_size_limit='10M'
 
 skipif config 3node-tenant
-statement error unimplemented
+statement error cannot use ALTER TENANT to change cluster settings in system tenant
+ALTER TENANT 1 SET CLUSTER SETTING server.mem_profile.total_dump_size_limit='10M'
+
+skipif config 3node-tenant
+statement ok
+SELECT crdb_internal.create_tenant(10)
+
+skipif config 3node-tenant
+statement ok
 ALTER TENANT 10 SET CLUSTER SETTING server.mem_profile.total_dump_size_limit='10M'
 
 skipif config 3node-tenant
-statement error unimplemented
+statement ok
 ALTER TENANT ALL SET CLUSTER SETTING server.mem_profile.total_dump_size_limit='10M'
 
 skipif config 3node-tenant
-statement error unimplemented
+statement ok
 ALTER TENANT 10 RESET CLUSTER SETTING server.mem_profile.total_dump_size_limit
 
 skipif config 3node-tenant
-statement error unimplemented
+statement ok
 ALTER TENANT ALL RESET CLUSTER SETTING server.mem_profile.total_dump_size_limit
 
+skipif config 3node-tenant
 statement error unimplemented
 SHOW CLUSTER SETTING server.mem_profile.total_dump_size_limit FOR TENANT 10
 
+onlyif config 3node-tenant
+statement error unimplemented
+SHOW CLUSTER SETTINGS FOR TENANT 10
+
+skipif config 3node-tenant
 statement error unimplemented
 SHOW CLUSTER SETTINGS FOR TENANT 10
 
+skipif config 3node-tenant
 statement error unimplemented
 SHOW PUBLIC CLUSTER SETTINGS FOR TENANT 10
 
+skipif config 3node-tenant
 statement error unimplemented
 SHOW ALL CLUSTER SETTINGS FOR TENANT 10

pkg/sql/tenant_settings.go

@@ -14,13 +14,17 @@ import (
 	"context"
 	"strings"
 
+	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
+	"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
 	"github.com/cockroachdb/errors"
 )
 
@@ -40,7 +44,33 @@ type alterTenantSetClusterSettingNode struct {
 func (p *planner) AlterTenantSetClusterSetting(
 	ctx context.Context, n *tree.AlterTenantSetClusterSetting,
 ) (planNode, error) {
+	// Changing cluster settings for other tenants is a more
+	// privileged operation than changing local cluster settings. So we
+	// shouldn't be allowing with just the role option
+	// MODIFYCLUSTERSETTINGS.
+	//
+	// TODO(knz): Using admin authz for now; we may want to introduce a
+	// more specific role option later.
+	if err := p.RequireAdminRole(ctx, "change a tenant cluster setting"); err != nil {
+		return nil, err
+	}
+	// Error out if we're trying to call this from a non-system tenant.
+	if !p.execCfg.Codec.ForSystemTenant() {
+		return nil, pgerror.Newf(pgcode.InsufficientPrivilege,
+			"ALTER TENANT can only be called by system operators")
+	}
+
 	name := strings.ToLower(n.Name)
+	st := p.EvalContext().Settings
+	v, ok := settings.Lookup(name, settings.LookupForLocalAccess, true /* forSystemTenant - checked above already */)
+	if !ok {
+		return nil, errors.Errorf("unknown cluster setting '%s'", name)
+	}
+	// Error out if we're trying to set a system-only variable.
+	if v.Class() == settings.SystemOnly {
+		return nil, pgerror.Newf(pgcode.InsufficientPrivilege,
+			"%s is a system-only setting and must be set in the admin tenant using SET CLUSTER SETTING", name)
+	}
 
 	var typedTenantID tree.TypedExpr
 	if !n.TenantAll {
@@ -53,30 +83,14 @@ func (p *planner) AlterTenantSetClusterSetting(
 		}
 	}
 
-	st := p.EvalContext().Settings
-	v, ok := settings.Lookup(name, settings.LookupForLocalAccess, p.ExecCfg().Codec.ForSystemTenant())
-	if !ok {
-		return nil, errors.Errorf("unknown cluster setting '%s'", name)
-	}
-
-	if err := checkPrivilegesForSetting(ctx, p, name, "set"); err != nil {
-		return nil, err
-	}
-
 	setting, ok := v.(settings.NonMaskedSetting)
 	if !ok {
 		return nil, errors.AssertionFailedf("expected writable setting, got %T", v)
 	}
 
-	// Error out if we're trying to call this from a non-system tenant or if
-	// we're trying to set a system-only variable.
-	if !p.execCfg.Codec.ForSystemTenant() {
-		return nil, pgerror.Newf(pgcode.InsufficientPrivilege,
-			"ALTER TENANT can only be called by system operators")
-	}
-	if setting.Class() == settings.SystemOnly {
-		return nil, pgerror.Newf(pgcode.InsufficientPrivilege,
-			"%s is a system-only setting and must be set at the host cluster level using SET CLUSTER SETTING", name)
+	// We don't support changing the version for another tenant just yet.
+	if _, isVersion := setting.(*settings.VersionSetting); isVersion {
+		return nil, unimplemented.NewWithIssue(77733, "cannot change the version of another tenant")
 	}
 
 	value, err := p.getAndValidateTypedClusterSetting(ctx, name, n.Value, setting)
@@ -92,14 +106,109 @@ func (p *planner) AlterTenantSetClusterSetting(
 }
 
 func (n *alterTenantSetClusterSettingNode) startExec(params runParams) error {
-	return unimplemented.NewWithIssue(73857,
-		`unimplemented: tenant-level cluster settings not supported`)
+	var tenantIDi uint64
+	var tenantID tree.Datum
+	if n.tenantID == nil {
+		// Processing for TENANT ALL.
+		// We will be writing rows with tenant_id = 0 in
+		// system.tenant_settings.
+		tenantID = tree.NewDInt(0)
+	} else {
+		// Case for TENANT <tenant_id>. We'll check that the provided
+		// tenant ID is non zero and refers to a tenant that exists in
+		// system.tenants.
+		var err error
+		tenantIDi, tenantID, err = resolveTenantID(params, n.tenantID)
+		if err != nil {
+			return err
+		}
+		if err := assertTenantExists(params, tenantID); err != nil {
+			return err
+		}
+	}
+
+	// Write the setting.
+	var reportedValue string
+	if n.value == nil {
+		// TODO(radu,knz): DEFAULT might be confusing, we really want to say "NO OVERRIDE"
+		reportedValue = "DEFAULT"
+		if _, err := params.p.execCfg.InternalExecutor.ExecEx(
+			params.ctx, "reset-tenant-setting", params.p.Txn(),
+			sessiondata.InternalExecutorOverride{User: security.RootUserName()},
+			"DELETE FROM system.tenant_settings WHERE tenant_id = $1 AND name = $2", tenantID, n.name,
+		); err != nil {
+			return err
+		}
+	} else {
+		reportedValue = tree.AsStringWithFlags(n.value, tree.FmtBareStrings)
+		value, err := n.value.Eval(params.p.EvalContext())
+		if err != nil {
+			return err
+		}
+		encoded, err := toSettingString(params.ctx, n.st, n.name, n.setting, value)
+		if err != nil {
+			return err
+		}
+		if _, err := params.p.execCfg.InternalExecutor.ExecEx(
+			params.ctx, "update-tenant-setting", params.p.Txn(),
+			sessiondata.InternalExecutorOverride{User: security.RootUserName()},
+			`UPSERT INTO system.tenant_settings (tenant_id, name, value, last_updated, value_type) VALUES ($1, $2, $3, now(), $4)`,
+			tenantID, n.name, encoded, n.setting.Typ(),
+		); err != nil {
+			return err
+		}
+	}
+
+	// Finally, log the event.
+	return params.p.logEvent(
+		params.ctx,
+		0, /* no target */
+		&eventpb.SetTenantClusterSetting{
+			SettingName: n.name,
+			Value:       reportedValue,
+			TenantId:    tenantIDi,
+			AllTenants:  tenantIDi == 0,
+		})
 }
 
 func (n *alterTenantSetClusterSettingNode) Next(_ runParams) (bool, error) { return false, nil }
 func (n *alterTenantSetClusterSettingNode) Values() tree.Datums            { return nil }
 func (n *alterTenantSetClusterSettingNode) Close(_ context.Context)        {}
 
+func resolveTenantID(params runParams, expr tree.TypedExpr) (uint64, tree.Datum, error) {
+	tenantIDd, err := expr.Eval(params.p.EvalContext())
+	if err != nil {
+		return 0, nil, err
+	}
+	tenantID, ok := tenantIDd.(*tree.DInt)
+	if !ok {
+		return 0, nil, errors.AssertionFailedf("expected int, got %T", tenantIDd)
+	}
+	if *tenantID == 0 {
+		return 0, nil, pgerror.Newf(pgcode.InvalidParameterValue, "tenant ID must be non-zero")
+	}
+	if roachpb.MakeTenantID(uint64(*tenantID)) == roachpb.SystemTenantID {
+		return 0, nil, errors.WithHint(pgerror.Newf(pgcode.InvalidParameterValue,
+			"cannot use ALTER TENANT to change cluster settings in system tenant"),
+			"Use a regular SET CLUSTER SETTING statement.")
+	}
+	return uint64(*tenantID), tenantIDd, nil
+}
+
+func assertTenantExists(params runParams, tenantID tree.Datum) error {
+	exists, err := params.p.ExecCfg().InternalExecutor.QueryRowEx(
+		params.ctx, "get-tenant", params.p.txn,
+		sessiondata.InternalExecutorOverride{User: security.RootUserName()},
+		`SELECT EXISTS(SELECT id FROM system.tenants WHERE id = $1)`, tenantID)
+	if err != nil {
+		return err
+	}
+	if exists[0] != tree.DBoolTrue {
+		return pgerror.Newf(pgcode.InvalidParameterValue, "no tenant found with ID %v", tenantID)
+	}
+	return nil
+}
+
 // ShowTenantClusterSetting shows the value of a cluster setting for a tenant.
 // Privileges: super user.
 func (p *planner) ShowTenantClusterSetting(