diff --git a/docs/generated/http/full.md b/docs/generated/http/full.md index e1ff55fa69e7..612b98e61e15 100644 --- a/docs/generated/http/full.md +++ b/docs/generated/http/full.md @@ -6122,7 +6122,7 @@ SettingsRequest inquires what are the current settings in the cluster. | Field | Type | Label | Description | Support status | | ----- | ---- | ----- | ----------- | -------------- | -| keys | [string](#cockroach.server.serverpb.SettingsRequest-string) | repeated | The array of setting names to retrieve. An empty keys array means "all". | [reserved](#support-status) | +| keys | [string](#cockroach.server.serverpb.SettingsRequest-string) | repeated | The array of setting keys or names to retrieve. An empty keys array means "all". | [reserved](#support-status) | | unredacted_values | [bool](#cockroach.server.serverpb.SettingsRequest-bool) | | Indicate whether to see unredacted setting values. This is opt-in so that a previous version `cockroach zip` does not start reporting values when this becomes active. For good security, the server only obeys this after it checks that the logger-in user has admin privilege. | [reserved](#support-status) | @@ -6174,6 +6174,7 @@ SettingsResponse is the response to SettingsRequest. | description | [string](#cockroach.server.serverpb.SettingsResponse-string) | | | [reserved](#support-status) | | public | [bool](#cockroach.server.serverpb.SettingsResponse-bool) | | | [reserved](#support-status) | | last_updated | [google.protobuf.Timestamp](#cockroach.server.serverpb.SettingsResponse-google.protobuf.Timestamp) | | | [reserved](#support-status) | +| name | [string](#cockroach.server.serverpb.SettingsResponse-string) | | for display purposes. | [reserved](#support-status) | diff --git a/pkg/ccl/backupccl/BUILD.bazel b/pkg/ccl/backupccl/BUILD.bazel index 094339155fc4..bd41bf7cce72 100644 --- a/pkg/ccl/backupccl/BUILD.bazel +++ b/pkg/ccl/backupccl/BUILD.bazel @@ -259,6 +259,7 @@ go_test( "//pkg/security/securitytest", "//pkg/security/username", "//pkg/server", + "//pkg/settings", "//pkg/settings/cluster", "//pkg/spanconfig", "//pkg/sql", diff --git a/pkg/ccl/backupccl/backup_test.go b/pkg/ccl/backupccl/backup_test.go index 4f179a01fd71..8d2719d7ec5f 100644 --- a/pkg/ccl/backupccl/backup_test.go +++ b/pkg/ccl/backupccl/backup_test.go @@ -6057,7 +6057,7 @@ func TestProtectedTimestampsDuringBackup(t *testing.T) { conn := tc.ServerConn(0) systemTenantRunner := sqlutils.MakeSQLRunner(conn) - setAndWaitForTenantReadOnlyClusterSetting(t, sql.SecondaryTenantZoneConfigsEnabled.Key(), + setAndWaitForTenantReadOnlyClusterSetting(t, sql.SecondaryTenantZoneConfigsEnabled.Name(), systemTenantRunner, ttSQLDB, tenantID, "true") // Run the test as the system tenant, and as the secondary tenant. diff --git a/pkg/ccl/backupccl/backupdest/backup_destination.go b/pkg/ccl/backupccl/backupdest/backup_destination.go index 2b28ef22daa3..d55d1525fa25 100644 --- a/pkg/ccl/backupccl/backupdest/backup_destination.go +++ b/pkg/ccl/backupccl/backupdest/backup_destination.go @@ -197,7 +197,7 @@ func ResolveDest( "Or, to take a full backup at a specific subdirectory, "+ "enable the deprecated syntax by switching the %q cluster setting to true; "+ "however, note this deprecated syntax will not be available in a future release.", - chosenSuffix, featureFullBackupUserSubdir.Key()) + chosenSuffix, featureFullBackupUserSubdir.Name()) } } // There's no full backup in the resolved subdirectory; therefore, we're conducting a full backup. diff --git a/pkg/ccl/backupccl/utils_test.go b/pkg/ccl/backupccl/utils_test.go index 92de4042c625..509da8eeeb77 100644 --- a/pkg/ccl/backupccl/utils_test.go +++ b/pkg/ccl/backupccl/utils_test.go @@ -36,6 +36,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/kv/kvserver" "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils" "github.com/cockroachdb/cockroach/pkg/sql/execinfra" @@ -391,7 +392,7 @@ func thresholdFromTrace(t *testing.T, traceString string) hlc.Timestamp { func setAndWaitForTenantReadOnlyClusterSetting( t *testing.T, - setting string, + settingName settings.SettingName, systemTenantRunner *sqlutils.SQLRunner, tenantRunner *sqlutils.SQLRunner, tenantID roachpb.TenantID, @@ -402,7 +403,7 @@ func setAndWaitForTenantReadOnlyClusterSetting( t, fmt.Sprintf( "ALTER TENANT [$1] SET CLUSTER SETTING %s = '%s'", - setting, + settingName, val, ), tenantID.ToUint64(), @@ -412,7 +413,7 @@ func setAndWaitForTenantReadOnlyClusterSetting( var currentVal string tenantRunner.QueryRow(t, fmt.Sprintf( - "SHOW CLUSTER SETTING %s", setting, + "SHOW CLUSTER SETTING %s", settingName, ), ).Scan(¤tVal) diff --git a/pkg/ccl/changefeedccl/authorization.go b/pkg/ccl/changefeedccl/authorization.go index 92769630497d..71ed652585e4 100644 --- a/pkg/ccl/changefeedccl/authorization.go +++ b/pkg/ccl/changefeedccl/authorization.go @@ -133,7 +133,7 @@ func authorizeUserToCreateChangefeed( return pgerror.Newf( pgcode.InsufficientPrivilege, `the %s privilege on all tables can only be used with external connection sinks. see cluster setting %s`, - privilege.CHANGEFEED, changefeedbase.RequireExternalConnectionSink.Key(), + privilege.CHANGEFEED, changefeedbase.RequireExternalConnectionSink.Name(), ) } } diff --git a/pkg/ccl/changefeedccl/changefeed_stmt.go b/pkg/ccl/changefeedccl/changefeed_stmt.go index f22082968879..be05cc727c26 100644 --- a/pkg/ccl/changefeedccl/changefeed_stmt.go +++ b/pkg/ccl/changefeedccl/changefeed_stmt.go @@ -634,7 +634,7 @@ func createChangefeedJobRecord( if !status.ChildMetricsEnabled.Get(&p.ExecCfg().Settings.SV) { p.BufferClientNotice(ctx, pgnotice.Newf( "%s is set to false, metrics will only be published to the '%s' label when it is set to true", - status.ChildMetricsEnabled.Key(), + status.ChildMetricsEnabled.Name(), scope, )) } diff --git a/pkg/ccl/multitenantccl/tenantcostclient/tenant_side_test.go b/pkg/ccl/multitenantccl/tenantcostclient/tenant_side_test.go index adff8443ddec..b599297b172c 100644 --- a/pkg/ccl/multitenantccl/tenantcostclient/tenant_side_test.go +++ b/pkg/ccl/multitenantccl/tenantcostclient/tenant_side_test.go @@ -1459,7 +1459,7 @@ func TestRUSettingsChanged(t *testing.T) { tenantcostmodel.ExternalIOIngressCostPerMiB, } for _, setting := range settings { - sysDB.Exec(t, fmt.Sprintf("ALTER TENANT ALL SET CLUSTER SETTING %s = $1", setting.Key()), setting.Default()*100) + sysDB.Exec(t, fmt.Sprintf("ALTER TENANT ALL SET CLUSTER SETTING %s = $1", setting.InternalKey()), setting.Default()*100) } // Check to make sure the cost of the query increased. Use SucceedsSoon diff --git a/pkg/ccl/oidcccl/authentication_oidc.go b/pkg/ccl/oidcccl/authentication_oidc.go index 005450991958..e53b2da15a58 100644 --- a/pkg/ccl/oidcccl/authentication_oidc.go +++ b/pkg/ccl/oidcccl/authentication_oidc.go @@ -520,7 +520,7 @@ var ConfigureOIDC = func( "attempting to extract SQL username from the payload using the claim key %s, issuer %s, and %s", claim, token.Issuer, - pgwire.ConnIdentityMapConf.Key(), + pgwire.ConnIdentityMapConf.Name(), ) } @@ -605,7 +605,7 @@ var ConfigureOIDC = func( } if len(acceptedUsernames) == 0 { - log.Errorf(ctx, "OIDC: failed to extract usernames from principals %v; check %s", tokenPrincipals, pgwire.ConnIdentityMapConf.Key()) + log.Errorf(ctx, "OIDC: failed to extract usernames from principals %v; check %s", tokenPrincipals, pgwire.ConnIdentityMapConf.Name()) http.Error(w, genericCallbackHTTPError, http.StatusInternalServerError) return } diff --git a/pkg/cli/gen.go b/pkg/cli/gen.go index 807d08d49687..f5457ae3c2f6 100644 --- a/pkg/cli/gen.go +++ b/pkg/cli/gen.go @@ -215,11 +215,11 @@ Output the list of cluster settings known to this binary. return s } - wrapDivSlug := func(s string) string { + wrapDivSlug := func(key settings.InternalKey, name settings.SettingName) string { if sqlExecCtx.TableDisplayFormat == clisqlexec.TableDisplayRawHTML { - return fmt.Sprintf(`
%s
`, slugify.Slugify(s), wrapCode(s)) + return fmt.Sprintf(`
%s
`, slugify.Slugify(string(key)), wrapCode(string(name))) } - return s + return string(name) } // Fill a Values struct with the defaults. @@ -227,11 +227,12 @@ Output the list of cluster settings known to this binary. settings.NewUpdater(&s.SV).ResetRemaining(context.Background()) var rows [][]string - for _, name := range settings.Keys(settings.ForSystemTenant) { - setting, ok := settings.LookupForLocalAccess(name, settings.ForSystemTenant) + for _, key := range settings.Keys(settings.ForSystemTenant) { + setting, ok := settings.LookupForLocalAccessByKey(key, settings.ForSystemTenant) if !ok { - panic(fmt.Sprintf("could not find setting %q", name)) + panic(fmt.Sprintf("could not find setting %q", key)) } + name := setting.Name() if excludeSystemSettings && setting.Class() == settings.SystemOnly { continue @@ -250,7 +251,7 @@ Output the list of cluster settings known to this binary. defaultVal = sm.SettingsListDefault() } else { defaultVal = setting.String(&s.SV) - if override, ok := upgrades.SettingsDefaultOverrides[name]; ok { + if override, ok := upgrades.SettingsDefaultOverrides[key]; ok { defaultVal = override } } @@ -261,7 +262,7 @@ Output the list of cluster settings known to this binary. settingDesc = html.EscapeString(settingDesc) alterRoleLink = `ALTER ROLE... SET` } - if strings.Contains(name, "sql.defaults") { + if strings.Contains(string(name), "sql.defaults") { settingDesc = fmt.Sprintf(`%s This cluster setting is being kept to preserve backwards-compatibility. This session variable default should now be configured using %s`, @@ -270,7 +271,7 @@ This session variable default should now be configured using %s`, ) } - row := []string{wrapDivSlug(name), typ, wrapCode(defaultVal), settingDesc} + row := []string{wrapDivSlug(key, name), typ, wrapCode(defaultVal), settingDesc} if showSettingClass { class := "unknown" switch setting.Class() { diff --git a/pkg/cloud/cloud_io.go b/pkg/cloud/cloud_io.go index e6ca7c67810f..dadc8a6865ac 100644 --- a/pkg/cloud/cloud_io.go +++ b/pkg/cloud/cloud_io.go @@ -166,7 +166,7 @@ func ResumingReaderRetryOnErrFnForSettings( retryTimeouts := retryConnectionTimedOut.Get(&st.SV) if retryTimeouts && sysutil.IsErrTimedOut(err) { - log.Warningf(ctx, "retrying connection timed out because %s = true", retryConnectionTimedOut.Key()) + log.Warningf(ctx, "retrying connection timed out because %s = true", retryConnectionTimedOut.Name()) return true } return false diff --git a/pkg/cloud/impl_registry.go b/pkg/cloud/impl_registry.go index 327dc3b0f4b4..439c7b19b921 100644 --- a/pkg/cloud/impl_registry.go +++ b/pkg/cloud/impl_registry.go @@ -66,10 +66,10 @@ func registerLimiterSettings(providerType cloudpb.ExternalStorageProvider) { sinkName = "nullsink" // keep the settings name pieces free of reserved keywords. } - readRateName := fmt.Sprintf("cloudstorage.%s.read.node_rate_limit", sinkName) - readBurstName := fmt.Sprintf("cloudstorage.%s.read.node_burst_limit", sinkName) - writeRateName := fmt.Sprintf("cloudstorage.%s.write.node_rate_limit", sinkName) - writeBurstName := fmt.Sprintf("cloudstorage.%s.write.node_burst_limit", sinkName) + readRateName := settings.InternalKey(fmt.Sprintf("cloudstorage.%s.read.node_rate_limit", sinkName)) + readBurstName := settings.InternalKey(fmt.Sprintf("cloudstorage.%s.read.node_burst_limit", sinkName)) + writeRateName := settings.InternalKey(fmt.Sprintf("cloudstorage.%s.write.node_rate_limit", sinkName)) + writeBurstName := settings.InternalKey(fmt.Sprintf("cloudstorage.%s.write.node_burst_limit", sinkName)) limiterSettings[providerType] = readAndWriteSettings{ read: rateAndBurstSettings{ @@ -265,7 +265,7 @@ type Limiters map[cloudpb.ExternalStorageProvider]rwLimiter func makeLimiter( ctx context.Context, sv *settings.Values, s rateAndBurstSettings, ) *quotapool.RateLimiter { - lim := quotapool.NewRateLimiter(s.rate.Key(), quotapool.Limit(0), 0) + lim := quotapool.NewRateLimiter(string(s.rate.Name()), quotapool.Limit(0), 0) fn := func(ctx context.Context) { rate := quotapool.Limit(s.rate.Get(sv)) if rate == 0 { diff --git a/pkg/kv/kvclient/kvtenant/connector.go b/pkg/kv/kvclient/kvtenant/connector.go index f8f0bad175d7..88efb4d82bd9 100644 --- a/pkg/kv/kvclient/kvtenant/connector.go +++ b/pkg/kv/kvclient/kvtenant/connector.go @@ -187,12 +187,12 @@ type connector struct { // receivedFirstAllTenantOverrides is set to true when the first batch of // all-tenant overrides has been received. receivedFirstAllTenantOverrides bool - allTenantOverrides map[string]settings.EncodedValue + allTenantOverrides map[settings.InternalKey]settings.EncodedValue // receivedFirstSpecificOverrides is set to true when the first batch of // tenant-specific overrides has been received. receivedFirstSpecificOverrides bool - specificOverrides map[string]settings.EncodedValue + specificOverrides map[settings.InternalKey]settings.EncodedValue // notifyCh is closed when there are changes to overrides. notifyCh chan struct{} @@ -288,8 +288,8 @@ func NewConnector(cfg ConnectorConfig, addrs []string) Connector { c.mu.nodeDescs = make(map[roachpb.NodeID]*roachpb.NodeDescriptor) c.mu.storeDescs = make(map[roachpb.StoreID]*roachpb.StoreDescriptor) c.mu.systemConfigChannels = make(map[chan<- struct{}]struct{}) - c.settingsMu.allTenantOverrides = make(map[string]settings.EncodedValue) - c.settingsMu.specificOverrides = make(map[string]settings.EncodedValue) + c.settingsMu.allTenantOverrides = make(map[settings.InternalKey]settings.EncodedValue) + c.settingsMu.specificOverrides = make(map[settings.InternalKey]settings.EncodedValue) c.settingsMu.notifyCh = make(chan struct{}) c.metadataMu.notifyCh = make(chan struct{}) return c diff --git a/pkg/kv/kvclient/kvtenant/setting_overrides.go b/pkg/kv/kvclient/kvtenant/setting_overrides.go index 9447c9a0ac21..0c27fe99b5a8 100644 --- a/pkg/kv/kvclient/kvtenant/setting_overrides.go +++ b/pkg/kv/kvclient/kvtenant/setting_overrides.go @@ -191,7 +191,7 @@ func (c *connector) processSettingsEvent( c.settingsMu.Lock() defer c.settingsMu.Unlock() - var m map[string]settings.EncodedValue + var m map[settings.InternalKey]settings.EncodedValue switch e.Precedence { case kvpb.TenantSettingsEvent_ALL_TENANTS_OVERRIDES: if !c.settingsMu.receivedFirstAllTenantOverrides && e.Incremental { @@ -230,9 +230,9 @@ func (c *connector) processSettingsEvent( for _, o := range e.Overrides { if o.Value == (settings.EncodedValue{}) { // Empty value indicates that the override is removed. - delete(m, o.Name) + delete(m, o.InternalKey) } else { - m[o.Name] = o.Value + m[o.InternalKey] = o.Value } } @@ -248,20 +248,20 @@ func (c *connector) processSettingsEvent( } // Overrides is part of the settingswatcher.OverridesMonitor interface. -func (c *connector) Overrides() (map[string]settings.EncodedValue, <-chan struct{}) { +func (c *connector) Overrides() (map[settings.InternalKey]settings.EncodedValue, <-chan struct{}) { c.settingsMu.Lock() defer c.settingsMu.Unlock() - res := make(map[string]settings.EncodedValue, len(c.settingsMu.allTenantOverrides)+len(c.settingsMu.specificOverrides)) + res := make(map[settings.InternalKey]settings.EncodedValue, len(c.settingsMu.allTenantOverrides)+len(c.settingsMu.specificOverrides)) // First copy the all-tenant overrides. - for name, val := range c.settingsMu.allTenantOverrides { - res[name] = val + for key, val := range c.settingsMu.allTenantOverrides { + res[key] = val } // Then copy the specific overrides (which can overwrite some all-tenant // overrides). - for name, val := range c.settingsMu.specificOverrides { - res[name] = val + for key, val := range c.settingsMu.specificOverrides { + res[key] = val } return res, c.settingsMu.notifyCh } diff --git a/pkg/kv/kvclient/kvtenant/setting_overrides_test.go b/pkg/kv/kvclient/kvtenant/setting_overrides_test.go index f04cae1a7b1a..0d4aed3270a6 100644 --- a/pkg/kv/kvclient/kvtenant/setting_overrides_test.go +++ b/pkg/kv/kvclient/kvtenant/setting_overrides_test.go @@ -130,10 +130,10 @@ func TestConnectorSettingOverrides(t *testing.T) { ch := expectSettings(t, c, "foo=default bar=default baz=default") - st := func(name, val string) kvpb.TenantSetting { + st := func(key settings.InternalKey, val string) kvpb.TenantSetting { return kvpb.TenantSetting{ - Name: name, - Value: settings.EncodedValue{Value: val}, + InternalKey: key, + Value: settings.EncodedValue{Value: val}, } } @@ -200,8 +200,8 @@ func waitForNotify(t *testing.T, ch <-chan struct{}) { func expectSettings(t *testing.T, c *connector, exp string) <-chan struct{} { t.Helper() - vars := []string{"foo", "bar", "baz"} - values := make(map[string]string) + vars := []settings.InternalKey{"foo", "bar", "baz"} + values := make(map[settings.InternalKey]string) for i := range vars { values[vars[i]] = "default" } diff --git a/pkg/kv/kvpb/api.proto b/pkg/kv/kvpb/api.proto index 4488c757647a..334fcc2f42b9 100644 --- a/pkg/kv/kvpb/api.proto +++ b/pkg/kv/kvpb/api.proto @@ -3258,7 +3258,7 @@ message TenantSettingsEvent { // TenantSetting contains the name and value of a tenant setting. message TenantSetting { - string name = 1; + string internal_key = 1 [(gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/settings.InternalKey"]; settings.EncodedValue value = 2 [(gogoproto.nullable) = false]; } diff --git a/pkg/kv/kvserver/store_snapshot.go b/pkg/kv/kvserver/store_snapshot.go index 44333c0a998a..19703c317faf 100644 --- a/pkg/kv/kvserver/store_snapshot.go +++ b/pkg/kv/kvserver/store_snapshot.go @@ -1124,8 +1124,8 @@ func (s *Store) throttleSnapshot( } return nil, errors.Wrapf( queueCtx.Err(), - "giving up during snapshot reservation due to %q", - snapshotReservationQueueTimeoutFraction.Key(), + "giving up during snapshot reservation due to cluster setting %q", + snapshotReservationQueueTimeoutFraction.Name(), ) case <-s.stopper.ShouldQuiesce(): return nil, errors.Errorf("stopped") diff --git a/pkg/multitenant/tenant_config.go b/pkg/multitenant/tenant_config.go index d5cc3422d671..ac30ae5c34c3 100644 --- a/pkg/multitenant/tenant_config.go +++ b/pkg/multitenant/tenant_config.go @@ -18,7 +18,7 @@ import ( // DefaultTenantSelectSettingName is the name of the setting that // configures the default tenant to use when a client does not specify // a specific tenant. -var DefaultTenantSelectSettingName = "server.controller.default_tenant" +const DefaultTenantSelectSettingName = "server.controller.default_tenant" // DefaultTenantSelect determines which tenant serves requests from // clients that do not specify explicitly the tenant they want to use. diff --git a/pkg/server/admin.go b/pkg/server/admin.go index a467757278c1..3e2f1c9f2715 100644 --- a/pkg/server/admin.go +++ b/pkg/server/admin.go @@ -2016,30 +2016,39 @@ func (s *adminServer) Settings( } } - var settingsKeys []string + // settingsKeys is the list of setting keys to retrieve. + settingsKeys := make([]settings.InternalKey, 0, len(req.Keys)) + for _, desiredSetting := range req.Keys { + // The API client can pass either names or internal keys through the API. + key, ok := settings.NameToKey(settings.SettingName(desiredSetting)) + if ok { + settingsKeys = append(settingsKeys, key) + } else { + settingsKeys = append(settingsKeys, settings.InternalKey(desiredSetting)) + } + } if !consoleSettingsOnly { - settingsKeys = req.Keys if len(settingsKeys) == 0 { settingsKeys = settings.Keys(settings.ForSystemTenant) } } else { - - if len(req.Keys) == 0 { + if len(settingsKeys) == 0 { settingsKeys = settings.ConsoleKeys() } else { - settingsKeys = []string{} - for _, k := range req.Keys { + newSettingsKeys := make([]settings.InternalKey, 0, len(settings.ConsoleKeys())) + for _, k := range settingsKeys { if slices.Contains(settings.ConsoleKeys(), k) { - settingsKeys = append(settingsKeys, k) + newSettingsKeys = append(settingsKeys, k) } } + settingsKeys = newSettingsKeys } } // Read the system.settings table to determine the settings for which we have // explicitly set values -- the in-memory SV has the set and default values // flattened for quick reads, but we'd only need the non-defaults for comparison. - alteredSettings := make(map[string]*time.Time) + alteredSettings := make(map[settings.InternalKey]*time.Time) if it, err := s.internalExecutor.QueryIteratorEx( ctx, "read-setting", nil, /* txn */ sessiondata.RootUserSessionDataOverride, @@ -2050,9 +2059,9 @@ func (s *adminServer) Settings( var ok bool for ok, err = it.Next(ctx); ok; ok, err = it.Next(ctx) { row := it.Cur() - name := string(tree.MustBeDString(row[0])) + key := settings.InternalKey(tree.MustBeDString(row[0])) lastUpdated := row[1].(*tree.DTimestamp) - alteredSettings[name] = &lastUpdated.Time + alteredSettings[key] = &lastUpdated.Time } if err != nil { // No need to clear AlteredSettings map since we only make best @@ -2066,9 +2075,9 @@ func (s *adminServer) Settings( var v settings.Setting var ok bool if redactValues { - v, ok = settings.LookupForReporting(k, settings.ForSystemTenant) + v, ok = settings.LookupForReportingByKey(k, settings.ForSystemTenant) } else { - v, ok = settings.LookupForLocalAccess(k, settings.ForSystemTenant) + v, ok = settings.LookupForLocalAccessByKey(k, settings.ForSystemTenant) } if !ok { continue @@ -2077,8 +2086,9 @@ func (s *adminServer) Settings( if val, ok := alteredSettings[k]; ok { altered = val } - resp.KeyValues[k] = serverpb.SettingsResponse_Value{ + resp.KeyValues[string(k)] = serverpb.SettingsResponse_Value{ Type: v.Typ(), + Name: string(v.Name()), // Note: v.String() redacts the values if the purpose is not "LocalAccess". Value: v.String(&s.st.SV), Description: v.Description(), diff --git a/pkg/server/application_api/config_test.go b/pkg/server/application_api/config_test.go index f52d7b8915ec..a2cafe2232b3 100644 --- a/pkg/server/application_api/config_test.go +++ b/pkg/server/application_api/config_test.go @@ -51,8 +51,8 @@ func TestAdminAPISettings(t *testing.T) { st := s.ClusterSettings() allKeys := settings.Keys(settings.ForSystemTenant) - checkSetting := func(t *testing.T, k string, v serverpb.SettingsResponse_Value) { - ref, ok := settings.LookupForReporting(k, settings.ForSystemTenant) + checkSetting := func(t *testing.T, k settings.InternalKey, v serverpb.SettingsResponse_Value) { + ref, ok := settings.LookupForReportingByKey(k, settings.ForSystemTenant) if !ok { t.Fatalf("%s: not found after initial lookup", k) } @@ -106,7 +106,7 @@ func TestAdminAPISettings(t *testing.T) { t.Fatalf("expected %d keys, got %d", len(allKeys), len(resp.KeyValues)) } for _, k := range allKeys { - if _, ok := resp.KeyValues[k]; !ok { + if _, ok := resp.KeyValues[string(k)]; !ok { t.Fatalf("expected key %s not found in response", k) } } @@ -122,7 +122,7 @@ func TestAdminAPISettings(t *testing.T) { } } - checkSetting(t, k, v) + checkSetting(t, settings.InternalKey(k), v) } if !seenRef { @@ -137,7 +137,7 @@ func TestAdminAPISettings(t *testing.T) { // type and description must match. for _, k := range allKeys { q := make(url.Values) - q.Add("keys", k) + q.Add("keys", string(k)) url := "settings?" + q.Encode() if err := srvtestutils.GetAdminJSONProto(s, url, &resp); err != nil { t.Fatalf("%s: %v", k, err) @@ -145,7 +145,7 @@ func TestAdminAPISettings(t *testing.T) { if len(resp.KeyValues) != 1 { t.Fatalf("%s: expected 1 response, got %d", k, len(resp.KeyValues)) } - v, ok := resp.KeyValues[k] + v, ok := resp.KeyValues[string(k)] if !ok { t.Fatalf("%s: response does not contain key", k) } @@ -218,7 +218,7 @@ func TestAdminAPISettings(t *testing.T) { } require.True(t, len(resp.KeyValues) == len(consoleKeys)) for k := range resp.KeyValues { - require.True(t, slices.Contains(consoleKeys, k)) + require.True(t, slices.Contains(consoleKeys, settings.InternalKey(k))) } // Non-admin with VIEWACTIVITY and not VIEWCLUSTERSETTING permission requesting specific cluster setting diff --git a/pkg/server/diagnostics/reporter.go b/pkg/server/diagnostics/reporter.go index 607a8b5c8106..4fa61cd538b4 100644 --- a/pkg/server/diagnostics/reporter.go +++ b/pkg/server/diagnostics/reporter.go @@ -213,9 +213,9 @@ func (r *Reporter) CreateReport( var ok bool for ok, err = it.Next(ctx); ok; ok, err = it.Next(ctx) { row := it.Cur() - name := string(tree.MustBeDString(row[0])) - info.AlteredSettings[name] = settings.RedactedValue( - name, &r.Settings.SV, r.TenantID == roachpb.SystemTenantID, + internalKey := string(tree.MustBeDString(row[0])) + info.AlteredSettings[internalKey] = settings.RedactedValue( + settings.InternalKey(internalKey), &r.Settings.SV, r.TenantID == roachpb.SystemTenantID, ) } if err != nil { diff --git a/pkg/server/node.go b/pkg/server/node.go index 0478c7c6e16e..bae8ee804698 100644 --- a/pkg/server/node.go +++ b/pkg/server/node.go @@ -2312,7 +2312,7 @@ func (n *Node) getVersionSettingWithUpdateCh( defer n.versionUpdateMu.Unlock() setting := kvpb.TenantSetting{ - Name: clusterversion.KeyVersionSetting, + InternalKey: clusterversion.KeyVersionSetting, Value: settings.EncodedValue{ Type: settings.VersionSettingValueType, Value: n.versionUpdateMu.encodedVersion, diff --git a/pkg/server/serverpb/admin.proto b/pkg/server/serverpb/admin.proto index 2791d16903b2..0782a843d5cb 100644 --- a/pkg/server/serverpb/admin.proto +++ b/pkg/server/serverpb/admin.proto @@ -597,7 +597,7 @@ message DecommissionStatusResponse { // SettingsRequest inquires what are the current settings in the cluster. message SettingsRequest { - // The array of setting names to retrieve. + // The array of setting keys or names to retrieve. // An empty keys array means "all". repeated string keys = 1; @@ -617,6 +617,7 @@ message SettingsResponse { string description = 3; bool public = 4; google.protobuf.Timestamp last_updated = 5 [(gogoproto.nullable) = true, (gogoproto.stdtime) = true]; + string name = 6; // for display purposes. } map key_values = 1 [(gogoproto.nullable) = false]; } diff --git a/pkg/server/settings_cache.go b/pkg/server/settings_cache.go index 2e73ada82704..d3110d6ed6a9 100644 --- a/pkg/server/settings_cache.go +++ b/pkg/server/settings_cache.go @@ -144,14 +144,15 @@ func initializeCachedSettings( ) error { dec := settingswatcher.MakeRowDecoder(codec) for _, kv := range kvs { - settings, val, _, err := dec.DecodeRow(kv, nil /* alloc */) + settingKeyS, val, _, err := dec.DecodeRow(kv, nil /* alloc */) if err != nil { - return errors.Wrap(err, `while decoding settings data --this likely indicates the settings table structure or encoding has been altered; --skipping settings updates`) + return errors.WithHint(errors.Wrap(err, "while decoding settings data"), + "This likely indicates the settings table structure or encoding has been altered;"+ + " skipping settings updates.") } - if err := updater.Set(ctx, settings, val); err != nil { - log.Warningf(ctx, "setting %q to %v failed: %+v", settings, val, err) + settingKey := settings.InternalKey(settingKeyS) + if err := updater.Set(ctx, settingKey, val); err != nil { + log.Warningf(ctx, "setting %q to %v failed: %+v", settingKey, val, err) } } updater.ResetRemaining(ctx) diff --git a/pkg/server/settingswatcher/overrides.go b/pkg/server/settingswatcher/overrides.go index 79828dc7b2cf..6b775df7ddfd 100644 --- a/pkg/server/settingswatcher/overrides.go +++ b/pkg/server/settingswatcher/overrides.go @@ -26,5 +26,5 @@ type OverridesMonitor interface { // map from setting key to EncodedValue. Any settings that are // present must be set to the overridden value. It also returns a // channel that will be closed when the overrides are updated. - Overrides() (map[string]settings.EncodedValue, <-chan struct{}) + Overrides() (map[settings.InternalKey]settings.EncodedValue, <-chan struct{}) } diff --git a/pkg/server/settingswatcher/settings_watcher.go b/pkg/server/settingswatcher/settings_watcher.go index 312c9f4226fb..0bb03f84b58d 100644 --- a/pkg/server/settingswatcher/settings_watcher.go +++ b/pkg/server/settingswatcher/settings_watcher.go @@ -54,8 +54,8 @@ type SettingsWatcher struct { syncutil.Mutex updater settings.Updater - values map[string]settingsValue - overrides map[string]settings.EncodedValue + values map[settings.InternalKey]settingsValue + overrides map[settings.InternalKey]settings.EncodedValue // storageClusterVersion is the cache of the storage cluster version // inside secondary tenants. It will be uninitialized in a system // tenant. @@ -139,13 +139,13 @@ func (s *SettingsWatcher) Start(ctx context.Context) error { } } - s.mu.values = make(map[string]settingsValue) + s.mu.values = make(map[settings.InternalKey]settingsValue) if s.overridesMonitor != nil { // Initialize the overrides. We want to do this before processing // the settings table, otherwise we could see temporary // transitions to the value in the table. - s.mu.overrides = make(map[string]settings.EncodedValue) + s.mu.overrides = make(map[settings.InternalKey]settings.EncodedValue) // Wait for the overrides monitor to be ready, which also ensures // it has received initial data from the KV layer. if err := s.overridesMonitor.WaitForStart(ctx); err != nil { @@ -247,7 +247,7 @@ func (s *SettingsWatcher) handleKV( ctx context.Context, kv *kvpb.RangeFeedValue, ) rangefeedbuffer.Event { var alloc tree.DatumAlloc - name, val, tombstone, err := s.dec.DecodeRow(roachpb.KeyValue{ + settingKeyS, val, tombstone, err := s.dec.DecodeRow(roachpb.KeyValue{ Key: kv.Key, Value: kv.Value, }, &alloc) @@ -257,20 +257,21 @@ func (s *SettingsWatcher) handleKV( logcrash.ReportOrPanic(ctx, &s.settings.SV, "%w", err) return nil } + settingKey := settings.InternalKey(settingKeyS) if !s.codec.ForSystemTenant() { - setting, ok := settings.LookupForLocalAccess(name, s.codec.ForSystemTenant()) + setting, ok := settings.LookupForLocalAccessByKey(settingKey, s.codec.ForSystemTenant()) if !ok { - log.Warningf(ctx, "unknown setting %s, skipping update", redact.Safe(name)) + log.Warningf(ctx, "unknown setting %s, skipping update", settingKey) return nil } if setting.Class() != settings.TenantWritable { - log.Warningf(ctx, "ignoring read-only setting %s", redact.Safe(name)) + log.Warningf(ctx, "ignoring read-only setting %s", settingKey) return nil } } - s.maybeSet(ctx, name, settingsValue{ + s.maybeSet(ctx, settingKey, settingsValue{ val: val, ts: kv.Value.Timestamp, tombstone: tombstone, @@ -283,7 +284,9 @@ func (s *SettingsWatcher) handleKV( // maybeSet will update the stored value and the corresponding setting // in response to a kv event, assuming that event is new. -func (s *SettingsWatcher) maybeSet(ctx context.Context, name string, sv settingsValue) { +func (s *SettingsWatcher) maybeSet( + ctx context.Context, key settings.InternalKey, sv settingsValue, +) { s.mu.Lock() defer s.mu.Unlock() // Skip updates which have an earlier timestamp to avoid regressing on the @@ -292,19 +295,19 @@ func (s *SettingsWatcher) maybeSet(ctx context.Context, name string, sv settings // the underlying rangefeed restarts. When that happens, we'll construct a // new settings updater and expect to re-process every setting which is // currently set. - if existing, ok := s.mu.values[name]; ok && sv.ts.Less(existing.ts) { + if existing, ok := s.mu.values[key]; ok && sv.ts.Less(existing.ts) { return } - _, hasOverride := s.mu.overrides[name] - s.mu.values[name] = sv + _, hasOverride := s.mu.overrides[key] + s.mu.values[key] = sv if sv.tombstone { // This event corresponds to a deletion. if !hasOverride { - s.setDefaultLocked(ctx, name) + s.setDefaultLocked(ctx, key) } } else { if !hasOverride { - s.setLocked(ctx, name, sv.val, settings.OriginExplicitlySet) + s.setLocked(ctx, key, sv.val, settings.OriginExplicitlySet) } } } @@ -322,7 +325,10 @@ const versionSettingKey = "version" // set the current value of a setting. func (s *SettingsWatcher) setLocked( - ctx context.Context, key string, val settings.EncodedValue, origin settings.ValueOrigin, + ctx context.Context, + key settings.InternalKey, + val settings.EncodedValue, + origin settings.ValueOrigin, ) { // Both the system tenant and secondary tenants no longer use this code // path to propagate cluster version changes (they rely on @@ -353,8 +359,8 @@ func (s *SettingsWatcher) setLocked( } // setDefaultLocked sets a setting to its default value. -func (s *SettingsWatcher) setDefaultLocked(ctx context.Context, key string) { - setting, ok := settings.LookupForLocalAccess(key, s.codec.ForSystemTenant()) +func (s *SettingsWatcher) setDefaultLocked(ctx context.Context, key settings.InternalKey) { + setting, ok := settings.LookupForLocalAccessByKey(key, s.codec.ForSystemTenant()) if !ok { log.Warningf(ctx, "failed to find setting %s, skipping update", redact.Safe(key)) return @@ -369,7 +375,7 @@ func (s *SettingsWatcher) setDefaultLocked(ctx context.Context, key string) { // updateOverrides updates the overrides map and updates any settings // accordingly. func (s *SettingsWatcher) updateOverrides(ctx context.Context) (updateCh <-chan struct{}) { - var newOverrides map[string]settings.EncodedValue + var newOverrides map[settings.InternalKey]settings.EncodedValue newOverrides, updateCh = s.overridesMonitor.Overrides() s.mu.Lock() @@ -434,10 +440,10 @@ func (s *SettingsWatcher) SetTestingKnobs(knobs *rangefeedcache.TestingKnobs) { } // IsOverridden implements cluster.OverridesInformer. -func (s *SettingsWatcher) IsOverridden(settingName string) bool { +func (s *SettingsWatcher) IsOverridden(settingKey settings.InternalKey) bool { s.mu.Lock() defer s.mu.Unlock() - _, exists := s.mu.overrides[settingName] + _, exists := s.mu.overrides[settingKey] return exists } diff --git a/pkg/server/settingswatcher/settings_watcher_external_test.go b/pkg/server/settingswatcher/settings_watcher_external_test.go index 07094f07c964..b9bdc54936e2 100644 --- a/pkg/server/settingswatcher/settings_watcher_external_test.go +++ b/pkg/server/settingswatcher/settings_watcher_external_test.go @@ -240,19 +240,19 @@ func TestSettingsWatcherWithOverrides(t *testing.T) { st := cluster.MakeTestingClusterSettings() f, err := rangefeed.NewFactory(stopper, kvDB, st, &rangefeed.TestingKnobs{}) require.NoError(t, err) - w := settingswatcher.NewWithOverrides(ts.Clock(), keys.SystemSQLCodec, st, f, stopper, m, nil) + w := settingswatcher.NewWithOverrides(ts.Clock(), ts.Codec(), st, f, stopper, m, nil) require.NoError(t, w.Start(ctx)) - expect := func(setting, value string) { + expect := func(setting settings.InternalKey, value string) { t.Helper() - s, ok := settings.LookupForLocalAccess(setting, settings.ForSystemTenant) + s, ok := settings.LookupForLocalAccessByKey(setting, settings.ForSystemTenant) require.True(t, ok) require.Equal(t, value, s.String(&st.SV)) } - expectSoon := func(setting, value string) { + expectSoon := func(setting settings.InternalKey, value string) { t.Helper() - s, ok := settings.LookupForLocalAccess(setting, settings.ForSystemTenant) + s, ok := settings.LookupForLocalAccessByKey(setting, settings.ForSystemTenant) require.True(t, ok) testutils.SucceedsSoon(t, func() error { if actual := s.String(&st.SV); actual != value { @@ -317,7 +317,7 @@ type testingOverrideMonitor struct { mu struct { syncutil.Mutex ch chan struct{} - overrides map[string]settings.EncodedValue + overrides map[settings.InternalKey]settings.EncodedValue } } @@ -326,7 +326,7 @@ var _ settingswatcher.OverridesMonitor = (*testingOverrideMonitor)(nil) func newTestingOverrideMonitor() *testingOverrideMonitor { m := &testingOverrideMonitor{} m.mu.ch = make(chan struct{}) - m.mu.overrides = make(map[string]settings.EncodedValue) + m.mu.overrides = make(map[settings.InternalKey]settings.EncodedValue) return m } @@ -337,7 +337,7 @@ func (m *testingOverrideMonitor) notify() { m.mu.ch = make(chan struct{}) } -func (m *testingOverrideMonitor) set(key string, val string, valType string) { +func (m *testingOverrideMonitor) set(key settings.InternalKey, val string, valType string) { m.mu.Lock() defer m.mu.Unlock() @@ -347,7 +347,7 @@ func (m *testingOverrideMonitor) set(key string, val string, valType string) { } } -func (m *testingOverrideMonitor) unset(key string) { +func (m *testingOverrideMonitor) unset(key settings.InternalKey) { m.mu.Lock() defer m.mu.Unlock() delete(m.mu.overrides, key) @@ -359,10 +359,13 @@ func (m *testingOverrideMonitor) WaitForStart(ctx context.Context) error { } // Overrides is part of the settingswatcher.OverridesMonitor interface. -func (m *testingOverrideMonitor) Overrides() (map[string]settings.EncodedValue, <-chan struct{}) { +func (m *testingOverrideMonitor) Overrides() ( + map[settings.InternalKey]settings.EncodedValue, + <-chan struct{}, +) { m.mu.Lock() defer m.mu.Unlock() - res := make(map[string]settings.EncodedValue) + res := make(map[settings.InternalKey]settings.EncodedValue) for k, v := range m.mu.overrides { res[k] = v } @@ -438,7 +441,7 @@ func TestOverflowRestart(t *testing.T) { // two settings do not match. It generally gets used with SucceeedsSoon. func CheckSettingsValuesMatch(t *testing.T, a, b *cluster.Settings) error { for _, k := range settings.Keys(false /* forSystemTenant */) { - s, ok := settings.LookupForLocalAccess(k, false /* forSystemTenant */) + s, ok := settings.LookupForLocalAccessByKey(k, false /* forSystemTenant */) require.True(t, ok) if s.Class() == settings.SystemOnly { continue diff --git a/pkg/server/tenantsettingswatcher/BUILD.bazel b/pkg/server/tenantsettingswatcher/BUILD.bazel index 85fe7c877e84..90242793f6f9 100644 --- a/pkg/server/tenantsettingswatcher/BUILD.bazel +++ b/pkg/server/tenantsettingswatcher/BUILD.bazel @@ -48,7 +48,6 @@ go_test( deps = [ "//pkg/base", "//pkg/clusterversion", - "//pkg/keys", "//pkg/kv/kvpb", "//pkg/roachpb", "//pkg/security/securityassets", diff --git a/pkg/server/tenantsettingswatcher/overrides_store.go b/pkg/server/tenantsettingswatcher/overrides_store.go index 59ecf9433a5c..351b64c9ce11 100644 --- a/pkg/server/tenantsettingswatcher/overrides_store.go +++ b/pkg/server/tenantsettingswatcher/overrides_store.go @@ -86,11 +86,11 @@ func (s *overridesStore) SetAll(allOverrides map[roachpb.TenantID][]kvpb.TenantS for tenantID, overrides := range allOverrides { // Make sure settings are sorted. sort.Slice(overrides, func(i, j int) bool { - return overrides[i].Name < overrides[j].Name + return overrides[i].InternalKey < overrides[j].InternalKey }) // Sanity check. for i := 1; i < len(overrides); i++ { - if overrides[i].Name == overrides[i-1].Name { + if overrides[i].InternalKey == overrides[i-1].InternalKey { panic("duplicate setting") } } @@ -137,8 +137,8 @@ func (s *overridesStore) SetTenantOverride(tenantID roachpb.TenantID, setting kv close(existing.changeCh) } after := make([]kvpb.TenantSetting, 0, len(before)+1) - // 1. Add all settings up to setting.Name. - for len(before) > 0 && before[0].Name < setting.Name { + // 1. Add all settings up to setting.InternalKey. + for len(before) > 0 && before[0].InternalKey < setting.InternalKey { after = append(after, before[0]) before = before[1:] } @@ -147,10 +147,10 @@ func (s *overridesStore) SetTenantOverride(tenantID roachpb.TenantID, setting kv after = append(after, setting) } // Skip any existing setting for this name. - if len(before) > 0 && before[0].Name == setting.Name { + if len(before) > 0 && before[0].InternalKey == setting.InternalKey { before = before[1:] } - // 3. Append all settings after setting.Name. + // 3. Append all settings after setting.InternalKey. after = append(after, before...) s.mu.tenants[tenantID] = newTenantOverrides(after) } diff --git a/pkg/server/tenantsettingswatcher/overrides_store_test.go b/pkg/server/tenantsettingswatcher/overrides_store_test.go index 51e127189e4b..8ea8d99ae381 100644 --- a/pkg/server/tenantsettingswatcher/overrides_store_test.go +++ b/pkg/server/tenantsettingswatcher/overrides_store_test.go @@ -29,9 +29,9 @@ func TestOverridesStore(t *testing.T) { s.Init() t1 := roachpb.MustMakeTenantID(1) t2 := roachpb.MustMakeTenantID(2) - st := func(name, val string) kvpb.TenantSetting { + st := func(key settings.InternalKey, val string) kvpb.TenantSetting { return kvpb.TenantSetting{ - Name: name, + InternalKey: key, Value: settings.EncodedValue{ Value: val, }, @@ -41,7 +41,7 @@ func TestOverridesStore(t *testing.T) { t.Helper() var vals []string for _, s := range o.overrides { - vals = append(vals, fmt.Sprintf("%s=%s", s.Name, s.Value.Value)) + vals = append(vals, fmt.Sprintf("%s=%s", s.InternalKey, s.Value.Value)) } if actual := strings.Join(vals, " "); actual != expected { t.Errorf("expected: %s; got: %s", expected, actual) diff --git a/pkg/server/tenantsettingswatcher/row_decoder.go b/pkg/server/tenantsettingswatcher/row_decoder.go index 741b57697bb5..d4966c05934e 100644 --- a/pkg/server/tenantsettingswatcher/row_decoder.go +++ b/pkg/server/tenantsettingswatcher/row_decoder.go @@ -14,6 +14,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv/kvpb" "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema" "github.com/cockroachdb/cockroach/pkg/sql/rowenc" @@ -61,7 +62,7 @@ func (d *RowDecoder) DecodeRow( // We do not use MustMakeTenantID because we want to tolerate the 0 value. tenantID := roachpb.TenantID{InternalValue: uint64(tree.MustBeDInt(keyVals[0].Datum))} var setting kvpb.TenantSetting - setting.Name = string(tree.MustBeDString(keyVals[1].Datum)) + setting.InternalKey = settings.InternalKey(tree.MustBeDString(keyVals[1].Datum)) if !kv.Value.IsPresent() { return tenantID, setting, true, nil } diff --git a/pkg/server/tenantsettingswatcher/row_decoder_test.go b/pkg/server/tenantsettingswatcher/row_decoder_test.go index 19154fe43e04..5af413c3ce31 100644 --- a/pkg/server/tenantsettingswatcher/row_decoder_test.go +++ b/pkg/server/tenantsettingswatcher/row_decoder_test.go @@ -15,12 +15,12 @@ import ( "testing" "github.com/cockroachdb/cockroach/pkg/base" - "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/server/tenantsettingswatcher" + "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/sql/catalog" + "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" - "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/stretchr/testify/require" ) @@ -31,12 +31,13 @@ func TestRowDecoder(t *testing.T) { defer leaktest.AfterTest(t)() ctx := context.Background() - tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{}) - defer tc.Stopper().Stop(ctx) + srv, db, kvDB := serverutils.StartServer(t, base.TestServerArgs{}) + defer srv.Stopper().Stop(ctx) + ts := srv.ApplicationLayer() - tdb := sqlutils.MakeSQLRunner(tc.ServerConn(0)) + tdb := sqlutils.MakeSQLRunner(db) - toSet := map[string]struct { + toSet := map[settings.InternalKey]struct { tenantID int val string typ string @@ -64,10 +65,10 @@ func TestRowDecoder(t *testing.T) { ) } - tableID, err := tc.Server(0).SystemTableIDResolver().(catalog.SystemTableIDResolver).LookupSystemTableID(ctx, "tenant_settings") + tableID, err := ts.SystemTableIDResolver().(catalog.SystemTableIDResolver).LookupSystemTableID(ctx, "tenant_settings") require.NoError(t, err) - k := keys.SystemSQLCodec.TablePrefix(uint32(tableID)) - rows, err := tc.Server(0).DB().Scan(ctx, k, k.PrefixEnd(), 0 /* maxRows */) + k := ts.Codec().TablePrefix(uint32(tableID)) + rows, err := kvDB.Scan(ctx, k, k.PrefixEnd(), 0 /* maxRows */) require.NoError(t, err) dec := tenantsettingswatcher.MakeRowDecoder() for _, row := range rows { @@ -79,11 +80,11 @@ func TestRowDecoder(t *testing.T) { tenantID, setting, tombstone, err := dec.DecodeRow(kv) require.NoError(t, err) require.False(t, tombstone) - if exp, ok := toSet[setting.Name]; ok { + if exp, ok := toSet[setting.InternalKey]; ok { require.Equal(t, exp.tenantID, int(tenantID.InternalValue)) require.Equal(t, exp.val, setting.Value.Value) require.Equal(t, exp.typ, setting.Value.Type) - delete(toSet, setting.Name) + delete(toSet, setting.InternalKey) } // Test the tombstone logic while we're here. @@ -92,7 +93,7 @@ func TestRowDecoder(t *testing.T) { require.NoError(t, err) require.True(t, tombstone) require.Equal(t, tenantID, tombstoneTenantID) - require.Equal(t, setting.Name, tombstoneSetting.Name) + require.Equal(t, setting.InternalKey, tombstoneSetting.InternalKey) require.Zero(t, tombstoneSetting.Value.Value) require.Zero(t, tombstoneSetting.Value.Type) } diff --git a/pkg/server/tenantsettingswatcher/watcher_test.go b/pkg/server/tenantsettingswatcher/watcher_test.go index 60d30228f602..d3bb9a03ffce 100644 --- a/pkg/server/tenantsettingswatcher/watcher_test.go +++ b/pkg/server/tenantsettingswatcher/watcher_test.go @@ -24,8 +24,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/server/tenantsettingswatcher" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/catalog" + "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" - "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/stretchr/testify/require" ) @@ -34,24 +34,24 @@ func TestWatcher(t *testing.T) { defer leaktest.AfterTest(t)() ctx := context.Background() - tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{}) - defer tc.Stopper().Stop(ctx) + srv, db, _ := serverutils.StartServer(t, base.TestServerArgs{}) + defer srv.Stopper().Stop(ctx) + ts := srv.ApplicationLayer() - r := sqlutils.MakeSQLRunner(tc.ServerConn(0)) + r := sqlutils.MakeSQLRunner(db) r.Exec(t, `INSERT INTO system.tenant_settings (tenant_id, name, value, value_type) VALUES (0, 'foo', 'foo-all', 's'), (1, 'foo', 'foo-t1', 's'), (1, 'bar', 'bar-t1', 's'), (2, 'baz', 'baz-t2', 's')`) - s0 := tc.Server(0) w := tenantsettingswatcher.New( - s0.Clock(), - s0.ExecutorConfig().(sql.ExecutorConfig).RangeFeedFactory, - s0.Stopper(), - s0.ClusterSettings(), + ts.Clock(), + ts.ExecutorConfig().(sql.ExecutorConfig).RangeFeedFactory, + ts.Stopper(), + ts.ClusterSettings(), ) - err := w.Start(ctx, s0.SystemTableIDResolver().(catalog.SystemTableIDResolver)) + err := w.Start(ctx, ts.SystemTableIDResolver().(catalog.SystemTableIDResolver)) require.NoError(t, err) // WaitForStart should return immediately. err = w.WaitForStart(ctx) @@ -61,10 +61,10 @@ func TestWatcher(t *testing.T) { t.Helper() var vals []string for _, s := range overrides { - if s.Name == clusterversion.KeyVersionSetting { + if s.InternalKey == clusterversion.KeyVersionSetting { continue } - vals = append(vals, fmt.Sprintf("%s=%s", s.Name, s.Value.Value)) + vals = append(vals, fmt.Sprintf("%s=%s", s.InternalKey, s.Value.Value)) } if actual := strings.Join(vals, " "); actual != expected { t.Errorf("expected: %s; got: %s", expected, actual) diff --git a/pkg/settings/bool.go b/pkg/settings/bool.go index 0b45237d200b..4b3973564135 100644 --- a/pkg/settings/bool.go +++ b/pkg/settings/bool.go @@ -104,7 +104,9 @@ func (b *BoolSetting) WithPublic() *BoolSetting { } // RegisterBoolSetting defines a new setting with type bool. -func RegisterBoolSetting(class Class, key, desc string, defaultValue bool) *BoolSetting { +func RegisterBoolSetting( + class Class, key InternalKey, desc string, defaultValue bool, +) *BoolSetting { setting := &BoolSetting{defaultValue: defaultValue} register(class, key, desc, setting) return setting diff --git a/pkg/settings/byte_size.go b/pkg/settings/byte_size.go index 359e9f779990..cabb655b76cc 100644 --- a/pkg/settings/byte_size.go +++ b/pkg/settings/byte_size.go @@ -52,7 +52,7 @@ func (b *ByteSizeSetting) WithPublic() *ByteSizeSetting { // supplied validation function(s). If no validation functions are given, then // the non-negative int validation is performed. func RegisterByteSizeSetting( - class Class, key, desc string, defaultValue int64, validateFns ...func(int64) error, + class Class, key InternalKey, desc string, defaultValue int64, validateFns ...func(int64) error, ) *ByteSizeSetting { var validateFn = func(v int64) error { diff --git a/pkg/settings/cluster/cluster_settings.go b/pkg/settings/cluster/cluster_settings.go index d72564aec18e..e9ac4206c3ae 100644 --- a/pkg/settings/cluster/cluster_settings.go +++ b/pkg/settings/cluster/cluster_settings.go @@ -66,7 +66,7 @@ type Settings struct { // TODO(radu): move this functionality into settings.Values, provide a way to // obtain it along with the current value consistently. type OverridesInformer interface { - IsOverridden(settingName string) bool + IsOverridden(settingKey settings.InternalKey) bool } // TelemetryOptOut controls whether to opt out of telemetry (including Sentry) or not. diff --git a/pkg/settings/common.go b/pkg/settings/common.go index a7987626f540..4423109d643f 100644 --- a/pkg/settings/common.go +++ b/pkg/settings/common.go @@ -18,7 +18,7 @@ import ( // common implements basic functionality used by all setting types. type common struct { class Class - key string + key InternalKey description string visibility Visibility slot slotIdx @@ -33,7 +33,7 @@ type common struct { type slotIdx int32 // init must be called to initialize the fields that don't have defaults. -func (c *common) init(class Class, key string, description string, slot slotIdx) { +func (c *common) init(class Class, key InternalKey, description string, slot slotIdx) { c.class = class c.key = key c.description = description @@ -50,10 +50,14 @@ func (c common) Class() Class { return c.class } -func (c common) Key() string { +func (c common) InternalKey() InternalKey { return c.key } +func (c common) Name() SettingName { + return SettingName(c.key) +} + func (c common) Description() string { return c.description } @@ -119,7 +123,7 @@ func (c *common) SetOnChange(sv *Values, fn func(ctx context.Context)) { type internalSetting interface { NonMaskedSetting - init(class Class, key, description string, slot slotIdx) + init(class Class, key InternalKey, description string, slot slotIdx) isRetired() bool setToDefault(ctx context.Context, sv *Values) diff --git a/pkg/settings/duration.go b/pkg/settings/duration.go index d701d1d938d7..ca36ce228757 100644 --- a/pkg/settings/duration.go +++ b/pkg/settings/duration.go @@ -136,7 +136,8 @@ func (d *DurationSetting) WithPublic() *DurationSetting { // RegisterDurationSetting defines a new setting with type duration. func RegisterDurationSetting( class Class, - key, desc string, + key InternalKey, + desc string, defaultValue time.Duration, validateFns ...func(time.Duration) error, ) *DurationSetting { @@ -169,7 +170,11 @@ func RegisterDurationSetting( // public setting with type duration which requires an explicit unit when being // set. func RegisterPublicDurationSettingWithExplicitUnit( - class Class, key, desc string, defaultValue time.Duration, validateFn func(time.Duration) error, + class Class, + key InternalKey, + desc string, + defaultValue time.Duration, + validateFn func(time.Duration) error, ) *DurationSettingWithExplicitUnit { var fn func(time.Duration) error diff --git a/pkg/settings/enum.go b/pkg/settings/enum.go index fc2f76dc9353..5d96d37e9a31 100644 --- a/pkg/settings/enum.go +++ b/pkg/settings/enum.go @@ -124,7 +124,7 @@ func (e *EnumSetting) WithPublic() *EnumSetting { // RegisterEnumSetting defines a new setting with type int. func RegisterEnumSetting( - class Class, key, desc string, defaultValue string, enumValues map[int64]string, + class Class, key InternalKey, desc string, defaultValue string, enumValues map[int64]string, ) *EnumSetting { enumValuesLower := make(map[int64]string) var i int64 diff --git a/pkg/settings/float.go b/pkg/settings/float.go index 3d33f532e796..18d1fa145f2e 100644 --- a/pkg/settings/float.go +++ b/pkg/settings/float.go @@ -125,7 +125,11 @@ func (f *FloatSetting) WithPublic() *FloatSetting { // RegisterFloatSetting defines a new setting with type float. func RegisterFloatSetting( - class Class, key, desc string, defaultValue float64, validateFns ...func(float64) error, + class Class, + key InternalKey, + desc string, + defaultValue float64, + validateFns ...func(float64) error, ) *FloatSetting { var validateFn func(float64) error if len(validateFns) > 0 { diff --git a/pkg/settings/int.go b/pkg/settings/int.go index 5255f1441d57..108a96472c7c 100644 --- a/pkg/settings/int.go +++ b/pkg/settings/int.go @@ -117,7 +117,7 @@ func (i *IntSetting) setToDefault(ctx context.Context, sv *Values) { // RegisterIntSetting defines a new setting with type int with a // validation function. func RegisterIntSetting( - class Class, key, desc string, defaultValue int64, validateFns ...func(int64) error, + class Class, key InternalKey, desc string, defaultValue int64, validateFns ...func(int64) error, ) *IntSetting { var composed func(int64) error if len(validateFns) > 0 { diff --git a/pkg/settings/internal_test.go b/pkg/settings/internal_test.go index 08b7ab743b55..d3c52bb0256f 100644 --- a/pkg/settings/internal_test.go +++ b/pkg/settings/internal_test.go @@ -19,7 +19,7 @@ import ( var b = RegisterBoolSetting(SystemOnly, "b", "desc", true) -func TesIgnoreDefaults(t *testing.T) { +func TestIgnoreDefaults(t *testing.T) { ctx := context.Background() sv := &Values{} sv.Init(ctx, TestOpaque) @@ -27,11 +27,11 @@ func TesIgnoreDefaults(t *testing.T) { ignoreAllUpdates = true defer func() { ignoreAllUpdates = false }() u := NewUpdater(sv) - require.NoError(t, u.Set(ctx, b.Key(), EncodedValue{Value: EncodeBool(false), Type: "b"})) + require.NoError(t, u.Set(ctx, b.InternalKey(), EncodedValue{Value: EncodeBool(false), Type: "b"})) require.Equal(t, true, b.Get(sv)) ignoreAllUpdates = false u = NewUpdater(sv) - require.NoError(t, u.Set(ctx, b.Key(), EncodedValue{Value: EncodeBool(false), Type: "b"})) + require.NoError(t, u.Set(ctx, b.InternalKey(), EncodedValue{Value: EncodeBool(false), Type: "b"})) require.Equal(t, false, b.Get(sv)) } diff --git a/pkg/settings/masked.go b/pkg/settings/masked.go index 0f00febb81e6..37651cfdf2d4 100644 --- a/pkg/settings/masked.go +++ b/pkg/settings/masked.go @@ -33,9 +33,14 @@ func (s *maskedSetting) Visibility() Visibility { return s.setting.Visibility() } -// Key returns the key string for the underlying setting. -func (s *maskedSetting) Key() string { - return s.setting.Key() +// InternalKey returns the key string for the underlying setting. +func (s *maskedSetting) InternalKey() InternalKey { + return s.setting.InternalKey() +} + +// Name returns the name string for the underlying setting. +func (s *maskedSetting) Name() SettingName { + return s.setting.Name() } // Description returns the description string for the underlying setting. diff --git a/pkg/settings/protobuf.go b/pkg/settings/protobuf.go index 46329dbaa4bf..3d0e06156251 100644 --- a/pkg/settings/protobuf.go +++ b/pkg/settings/protobuf.go @@ -154,7 +154,7 @@ func (s *ProtobufSetting) UnmarshalFromJSON(jsonEncoded string) (protoutil.Messa // RegisterProtobufSetting defines a new setting with type protobuf. func RegisterProtobufSetting( - class Class, key, desc string, defaultValue protoutil.Message, + class Class, key InternalKey, desc string, defaultValue protoutil.Message, ) *ProtobufSetting { return RegisterValidatedProtobufSetting(class, key, desc, defaultValue, nil) } @@ -163,7 +163,8 @@ func RegisterProtobufSetting( // with a validation function. func RegisterValidatedProtobufSetting( class Class, - key, desc string, + key InternalKey, + desc string, defaultValue protoutil.Message, validateFn func(*Values, protoutil.Message) error, ) *ProtobufSetting { diff --git a/pkg/settings/registry.go b/pkg/settings/registry.go index 9cba09ee8b5a..fa4f9d313ac7 100644 --- a/pkg/settings/registry.go +++ b/pkg/settings/registry.go @@ -26,7 +26,7 @@ import ( // // registry should never be mutated after creation (except in tests), as it is // read concurrently by different callers. -var registry = make(map[string]internalSetting) +var registry = make(map[InternalKey]internalSetting) // slotTable stores the same settings as the registry, but accessible by the // slot index. @@ -35,7 +35,7 @@ var slotTable [MaxSettings]internalSetting // TestingSaveRegistry can be used in tests to save/restore the current // contents of the registry. func TestingSaveRegistry() func() { - var origRegistry = make(map[string]internalSetting) + var origRegistry = make(map[InternalKey]internalSetting) for k, v := range registry { origRegistry[k] = v } @@ -46,7 +46,7 @@ func TestingSaveRegistry() func() { // When a setting is removed, it should be added to this list so that we cannot // accidentally reuse its name, potentially mis-handling older values. -var retiredSettings = map[string]struct{}{ +var retiredSettings = map[InternalKey]struct{}{ // removed as of 2.0. "kv.gc.batch_size": {}, "kv.transaction.max_intents": {}, @@ -180,7 +180,7 @@ var retiredSettings = map[string]struct{}{ // cluster settings. In 22.2 and later, new session settings do not need an // associated sql.defaults cluster setting. Instead they can have their default // changed with ALTER ROLE ... SET. -var sqlDefaultSettings = map[string]struct{}{ +var sqlDefaultSettings = map[InternalKey]struct{}{ // PLEASE DO NOT ADD NEW SETTINGS TO THIS MAP. THANK YOU. "sql.defaults.cost_scans_with_default_col_size.enabled": {}, "sql.defaults.datestyle": {}, @@ -233,14 +233,14 @@ var sqlDefaultSettings = map[string]struct{}{ } // register adds a setting to the registry. -func register(class Class, key, desc string, s internalSetting) { +func register(class Class, key InternalKey, desc string, s internalSetting) { if _, ok := retiredSettings[key]; ok { panic(fmt.Sprintf("cannot reuse previously defined setting name: %s", key)) } if _, ok := registry[key]; ok { panic(fmt.Sprintf("setting already defined: %s", key)) } - if strings.Contains(key, "sql.defaults") { + if strings.Contains(string(key), "sql.defaults") { if _, ok := sqlDefaultSettings[key]; !ok { panic(fmt.Sprintf( "new sql.defaults cluster settings: %s is not needed now that `ALTER ROLE ... SET` syntax "+ @@ -275,8 +275,8 @@ func register(class Class, key, desc string, s internalSetting) { func NumRegisteredSettings() int { return len(registry) } // Keys returns a sorted string array with all the known keys. -func Keys(forSystemTenant bool) (res []string) { - res = make([]string, 0, len(registry)) +func Keys(forSystemTenant bool) (res []InternalKey) { + res = make([]InternalKey, 0, len(registry)) for k, v := range registry { if v.isRetired() { continue @@ -286,7 +286,7 @@ func Keys(forSystemTenant bool) (res []string) { } res = append(res, k) } - sort.Strings(res) + sort.Slice(res, func(i, j int) bool { return res[i] < res[j] }) return res } @@ -296,28 +296,46 @@ func Keys(forSystemTenant bool) (res []string) { // information, because it will be returned on `settings` endpoint for // users without VIEWCLUSTERSETTING or MODIFYCLUSTERSETTING permission, // but that have VIEWACTIVITY or VIEWACTIVITYREDACTED permissions. -func ConsoleKeys() (res []string) { - return []string{ - "cross_cluster_replication.enabled", - "keyvisualizer.enabled", - "keyvisualizer.sample_interval", - "sql.index_recommendation.drop_unused_duration", - "sql.insights.anomaly_detection.latency_threshold", - "sql.insights.high_retry_count.threshold", - "sql.insights.latency_threshold", - "sql.stats.automatic_collection.enabled", - "timeseries.storage.resolution_10s.ttl", - "timeseries.storage.resolution_30m.ttl", - "ui.display_timezone", - "version", - } +func ConsoleKeys() (res []InternalKey) { + return allConsoleKeys +} + +var allConsoleKeys = []InternalKey{ + "cross_cluster_replication.enabled", + "keyvisualizer.enabled", + "keyvisualizer.sample_interval", + "sql.index_recommendation.drop_unused_duration", + "sql.insights.anomaly_detection.latency_threshold", + "sql.insights.high_retry_count.threshold", + "sql.insights.latency_threshold", + "sql.stats.automatic_collection.enabled", + "timeseries.storage.resolution_10s.ttl", + "timeseries.storage.resolution_30m.ttl", + "ui.display_timezone", + "version", +} + +// NameToKey returns the key associated with a setting name. +func NameToKey(name SettingName) (InternalKey, bool) { + // TODO(...): improve this. + key := InternalKey(name) + return key, true } // LookupForLocalAccess returns a NonMaskedSetting by name. Used when a setting // is being retrieved for local processing within the cluster and not for // reporting; sensitive values are accessible. -func LookupForLocalAccess(name string, forSystemTenant bool) (NonMaskedSetting, bool) { - s, ok := registry[name] +func LookupForLocalAccess(name SettingName, forSystemTenant bool) (NonMaskedSetting, bool) { + // TODO(...): handle names different from keys. + key := InternalKey(name) + return LookupForLocalAccessByKey(key, forSystemTenant) +} + +// LookupForLocalAccessByKey returns a NonMaskedSetting by key. Used when a +// setting is being retrieved for local processing within the cluster and not +// for reporting; sensitive values are accessible. +func LookupForLocalAccessByKey(key InternalKey, forSystemTenant bool) (NonMaskedSetting, bool) { + s, ok := registry[key] if !ok { return nil, false } @@ -329,11 +347,21 @@ func LookupForLocalAccess(name string, forSystemTenant bool) (NonMaskedSetting, // LookupForReporting returns a Setting by name. Used when a setting is being // retrieved for reporting. +// For settings that are non-reportable, the returned Setting hides the current +// value (see Setting.String). +func LookupForReporting(name SettingName, forSystemTenant bool) (Setting, bool) { + // TODO(...): handle names different from keys. + key := InternalKey(name) + return LookupForReportingByKey(key, forSystemTenant) +} + +// LookupForReportingByKey returns a Setting by key. Used when a setting is being +// retrieved for reporting. // // For settings that are non-reportable, the returned Setting hides the current // value (see Setting.String). -func LookupForReporting(name string, forSystemTenant bool) (Setting, bool) { - s, ok := registry[name] +func LookupForReportingByKey(key InternalKey, forSystemTenant bool) (Setting, bool) { + s, ok := registry[key] if !ok { return nil, false } @@ -369,8 +397,8 @@ var ReadableTypes = map[string]string{ // is a string setting with an empty value); // - "" if the setting is not reportable; // - "" if there is no setting with this name. -func RedactedValue(name string, values *Values, forSystemTenant bool) string { - if setting, ok := LookupForReporting(name, forSystemTenant); ok { +func RedactedValue(key InternalKey, values *Values, forSystemTenant bool) string { + if setting, ok := LookupForReportingByKey(key, forSystemTenant); ok { return setting.String(values) } return "" diff --git a/pkg/settings/setting.go b/pkg/settings/setting.go index 1b3daae5e4da..86a983716206 100644 --- a/pkg/settings/setting.go +++ b/pkg/settings/setting.go @@ -15,6 +15,23 @@ import ( "fmt" ) +// SettingName represents the user-visible name of a cluster setting. +// The name is suitable for: +// - SHOW/SET CLUSTER SETTING. +// - inclusion in error messages. +// +// For internal storage, use the InternalKey type instead. +type SettingName string + +// InternalKey is the internal (storage) key for a cluster setting. +// The internal key is suitable for use with: +// - direct accesses to system.settings. +// - rangefeed logic associated with system.settings. +// - (in unit tests only) interchangeably with the name for SET CLUSTER SETTING. +// +// For user-visible displays, use the SettingName type instead. +type InternalKey string + // Setting is the interface exposing the metadata for a cluster setting. // // The values for the settings are stored separately in Values. This allows @@ -25,8 +42,21 @@ type Setting interface { // Class returns the scope of the setting in multi-tenant scenarios. Class() Class - // Key returns the name of the specific cluster setting. - Key() string + // InternalKey returns the internal key used to store the setting. + // To display the name of the setting (eg. in errors etc) or the + // SET/SHOW CLUSTER SETTING statements, use the Name() method instead. + // + // The internal key is suitable for use with: + // - direct accesses to system.settings. + // - rangefeed logic associated with system.settings. + // - (in unit tests only) interchangeably with the name for SET CLUSTER SETTING. + InternalKey() InternalKey + + // Name is the user-visible (display) name of the setting. + // The name is suitable for: + // - SHOW/SET CLUSTER SETTING. + // - inclusion in error messages. + Name() SettingName // Typ returns the short (1 char) string denoting the type of setting. Typ() string @@ -169,3 +199,12 @@ func (v ValueOrigin) String() string { } return [...]string{"default", "override", "external-override"}[v] } + +// SafeValue implements the redact.SafeValue interface. +func (ValueOrigin) SafeValue() {} + +// SafeValue implements the redact.SafeValue interface. +func (SettingName) SafeValue() {} + +// SafeValue implements the redact.SafeValue interface. +func (InternalKey) SafeValue() {} diff --git a/pkg/settings/settings_test.go b/pkg/settings/settings_test.go index 213e2aae8285..b541d574eb8a 100644 --- a/pkg/settings/settings_test.go +++ b/pkg/settings/settings_test.go @@ -221,7 +221,8 @@ func TestValidation(t *testing.T) { func TestIntrospection(t *testing.T) { require.Equal(t, "b", boolTA.Typ()) - require.Equal(t, "bool.t", boolTA.Key()) + require.Equal(t, settings.InternalKey("bool.t"), boolTA.InternalKey()) + require.Equal(t, settings.SettingName("bool.t"), boolTA.Name()) require.Equal(t, "desc", boolTA.Description()) require.Equal(t, settings.Reserved, boolTA.Visibility()) require.Equal(t, settings.SystemOnly, boolTA.Class()) @@ -351,9 +352,9 @@ func TestCache(t *testing.T) { t.Run("lookup-system", func(t *testing.T) { for _, s := range []settings.Setting{i1A, iVal, fA, fVal, dA, dVal, eA, mA, duA} { - result, ok := settings.LookupForLocalAccess(s.Key(), settings.ForSystemTenant) + result, ok := settings.LookupForLocalAccess(s.Name(), settings.ForSystemTenant) if !ok { - t.Fatalf("lookup(%s) failed", s.Key()) + t.Fatalf("lookup(%s) failed", s.Name()) } if result != s { t.Fatalf("expected %v, got %v", s, result) @@ -362,9 +363,9 @@ func TestCache(t *testing.T) { }) t.Run("lookup-tenant", func(t *testing.T) { for _, s := range []settings.Setting{i1A, fA, dA, duA} { - result, ok := settings.LookupForLocalAccess(s.Key(), false /* forSystemTenant */) + result, ok := settings.LookupForLocalAccess(s.Name(), false /* forSystemTenant */) if !ok { - t.Fatalf("lookup(%s) failed", s.Key()) + t.Fatalf("lookup(%s) failed", s.Name()) } if result != s { t.Fatalf("expected %v, got %v", s, result) @@ -373,9 +374,9 @@ func TestCache(t *testing.T) { }) t.Run("lookup-tenant-fail", func(t *testing.T) { for _, s := range []settings.Setting{iVal, fVal, dVal, eA, mA} { - _, ok := settings.LookupForLocalAccess(s.Key(), false /* forSystemTenant */) + _, ok := settings.LookupForLocalAccess(s.Name(), false /* forSystemTenant */) if ok { - t.Fatalf("lookup(%s) should have failed", s.Key()) + t.Fatalf("lookup(%s) should have failed", s.Name()) } } }) @@ -703,7 +704,7 @@ func TestIsReportable(t *testing.T) { func TestOnChangeWithMaxSettings(t *testing.T) { ctx := context.Background() // Register MaxSettings settings to ensure that no errors occur. - maxName, err := batchRegisterSettings(t, t.Name(), settings.MaxSettings-settings.NumRegisteredSettings()) + maxKey, maxName, err := batchRegisterSettings(t, t.Name(), settings.MaxSettings-settings.NumRegisteredSettings()) if err != nil { t.Fatalf("expected no error to register %d settings, but get error: %v", settings.MaxSettings, err) } @@ -723,7 +724,7 @@ func TestOnChangeWithMaxSettings(t *testing.T) { intSetting.SetOnChange(sv, func(ctx context.Context) { changes++ }) u := settings.NewUpdater(sv) - if err := u.Set(ctx, maxName, v(settings.EncodeInt(9), "i")); err != nil { + if err := u.Set(ctx, maxKey, v(settings.EncodeInt(9), "i")); err != nil { t.Fatal(err) } @@ -736,7 +737,7 @@ func TestMaxSettingsPanics(t *testing.T) { defer settings.TestingSaveRegistry()() // Register too many settings which will cause a panic which is caught and converted to an error. - _, err := batchRegisterSettings(t, t.Name(), + _, _, err := batchRegisterSettings(t, t.Name(), settings.MaxSettings-settings.NumRegisteredSettings()+1) expectedErr := "too many settings; increase MaxSettings" if !testutils.IsError(err, expectedErr) { @@ -745,7 +746,9 @@ func TestMaxSettingsPanics(t *testing.T) { } -func batchRegisterSettings(t *testing.T, keyPrefix string, count int) (name string, err error) { +func batchRegisterSettings( + t *testing.T, keyPrefix string, count int, +) (key settings.InternalKey, name settings.SettingName, err error) { defer func() { // Catch panic and convert it to an error. if r := recover(); r != nil { @@ -757,10 +760,11 @@ func batchRegisterSettings(t *testing.T, keyPrefix string, count int) (name stri } }() for i := 0; i < count; i++ { - name = fmt.Sprintf("%s_%3d", keyPrefix, i) - settings.RegisterIntSetting(settings.SystemOnly, name, "desc", 0) + key = settings.InternalKey(fmt.Sprintf("%s_%3d", keyPrefix, i)) + s := settings.RegisterIntSetting(settings.SystemOnly, key, "desc", 0) + name = s.Name() } - return name, err + return key, name, err } var overrideBool = settings.RegisterBoolSetting(settings.SystemOnly, "override.bool", "desc", true) diff --git a/pkg/settings/string.go b/pkg/settings/string.go index a78a60c19189..eb7fa3fc3b8e 100644 --- a/pkg/settings/string.go +++ b/pkg/settings/string.go @@ -107,14 +107,20 @@ func (s *StringSetting) WithPublic() *StringSetting { } // RegisterStringSetting defines a new setting with type string. -func RegisterStringSetting(class Class, key, desc string, defaultValue string) *StringSetting { +func RegisterStringSetting( + class Class, key InternalKey, desc string, defaultValue string, +) *StringSetting { return RegisterValidatedStringSetting(class, key, desc, defaultValue, nil) } // RegisterValidatedStringSetting defines a new setting with type string with a // validation function. func RegisterValidatedStringSetting( - class Class, key, desc string, defaultValue string, validateFn func(*Values, string) error, + class Class, + key InternalKey, + desc string, + defaultValue string, + validateFn func(*Values, string) error, ) *StringSetting { if validateFn != nil { if err := validateFn(nil, defaultValue); err != nil { diff --git a/pkg/settings/updater.go b/pkg/settings/updater.go index daf2db006dde..a051d702a851 100644 --- a/pkg/settings/updater.go +++ b/pkg/settings/updater.go @@ -60,7 +60,7 @@ func EncodeProtobuf(p protoutil.Message) string { type updater struct { sv *Values - m map[string]struct{} + m map[InternalKey]struct{} } // Updater is a helper for updating the in-memory settings. @@ -70,21 +70,21 @@ type updater struct { // wrapped atomic settings values as we go and note which settings were updated, // then set the rest to default in ResetRemaining(). type Updater interface { - Set(ctx context.Context, key string, value EncodedValue) error + Set(ctx context.Context, key InternalKey, value EncodedValue) error ResetRemaining(ctx context.Context) - SetValueOrigin(ctx context.Context, key string, origin ValueOrigin) + SetValueOrigin(ctx context.Context, key InternalKey, origin ValueOrigin) } // A NoopUpdater ignores all updates. type NoopUpdater struct{} // Set implements Updater. It is a no-op. -func (u NoopUpdater) Set(ctx context.Context, key string, value EncodedValue) error { return nil } +func (u NoopUpdater) Set(ctx context.Context, key InternalKey, value EncodedValue) error { return nil } // ResetRemaining implements Updater. It is a no-op. func (u NoopUpdater) ResetRemaining(context.Context) {} -func (u NoopUpdater) SetValueOrigin(ctx context.Context, key string, origin ValueOrigin) {} +func (u NoopUpdater) SetValueOrigin(ctx context.Context, key InternalKey, origin ValueOrigin) {} // NewUpdater makes an Updater. func NewUpdater(sv *Values) Updater { @@ -92,13 +92,13 @@ func NewUpdater(sv *Values) Updater { return NoopUpdater{} } return updater{ - m: make(map[string]struct{}, len(registry)), + m: make(map[InternalKey]struct{}, len(registry)), sv: sv, } } // Set attempts to parse and update a setting and notes that it was updated. -func (u updater) Set(ctx context.Context, key string, value EncodedValue) error { +func (u updater) Set(ctx context.Context, key InternalKey, value EncodedValue) error { d, ok := registry[key] if !ok { if _, ok := retiredSettings[key]; ok { @@ -111,7 +111,7 @@ func (u updater) Set(ctx context.Context, key string, value EncodedValue) error u.m[key] = struct{}{} if expected := d.Typ(); value.Type != expected { - return errors.Errorf("setting '%s' defined as type %s, not %s", key, expected, value.Type) + return errors.Errorf("setting '%s' defined as type %s, not %s", d.Name(), expected, value.Type) } switch setting := d.(type) { @@ -186,7 +186,7 @@ func (u updater) ResetRemaining(ctx context.Context) { } // SetValueOrigin sets the origin of the value of a given setting. -func (u updater) SetValueOrigin(ctx context.Context, key string, origin ValueOrigin) { +func (u updater) SetValueOrigin(ctx context.Context, key InternalKey, origin ValueOrigin) { d, ok := registry[key] if ok { u.sv.setValueOrigin(ctx, d.getSlot(), origin) diff --git a/pkg/settings/values.go b/pkg/settings/values.go index 4ad26ef58ea9..8e9bd0c7ac6a 100644 --- a/pkg/settings/values.go +++ b/pkg/settings/values.go @@ -95,7 +95,7 @@ func (c *valuesContainer) getGeneric(slot slotIdx) interface{} { func (c *valuesContainer) checkForbidden(slot slotIdx) bool { if c.forbidden[slot] { if buildutil.CrdbTestBuild { - panic(errors.AssertionFailedf("attempted to set forbidden setting %s", slotTable[slot].Key())) + panic(errors.AssertionFailedf("attempted to set forbidden setting %s", slotTable[slot].Name())) } return false } diff --git a/pkg/settings/version.go b/pkg/settings/version.go index b47bec458372..cfec2f4e0652 100644 --- a/pkg/settings/version.go +++ b/pkg/settings/version.go @@ -181,6 +181,6 @@ func (v *VersionSetting) setToDefault(ctx context.Context, sv *Values) {} // RegisterVersionSetting adds the provided version setting to the global // registry. -func RegisterVersionSetting(class Class, key, desc string, setting *VersionSetting) { +func RegisterVersionSetting(class Class, key InternalKey, desc string, setting *VersionSetting) { register(class, key, desc, setting) } diff --git a/pkg/sql/catalog/schematelemetry/schema_telemetry_test.go b/pkg/sql/catalog/schematelemetry/schema_telemetry_test.go index 46d1b271c904..74b1ce9d6bfe 100644 --- a/pkg/sql/catalog/schematelemetry/schema_telemetry_test.go +++ b/pkg/sql/catalog/schematelemetry/schema_telemetry_test.go @@ -64,7 +64,7 @@ var ( schematelemetrycontroller.SchemaTelemetryScheduleName) qSet = fmt.Sprintf(`SET CLUSTER SETTING %s = '* * * * *'`, - schematelemetrycontroller.SchemaTelemetryRecurrence.Key()) + schematelemetrycontroller.SchemaTelemetryRecurrence.InternalKey()) qJob = fmt.Sprintf(`SELECT %s()`, builtinconstants.CreateSchemaTelemetryJobBuiltinName) diff --git a/pkg/sql/catalog/systemschema/system.go b/pkg/sql/catalog/systemschema/system.go index ef853ac7074c..f662ba8b144b 100644 --- a/pkg/sql/catalog/systemschema/system.go +++ b/pkg/sql/catalog/systemschema/system.go @@ -88,6 +88,8 @@ CREATE TABLE system.zones ( SettingsTableSchema = ` CREATE TABLE system.settings ( + -- the internal key for the setting. The column is called 'name' for + -- historical reasons. name STRING NOT NULL, value STRING NOT NULL, "lastUpdated" TIMESTAMP NOT NULL DEFAULT now(), @@ -838,6 +840,8 @@ CREATE TABLE system.tenant_settings ( -- applies to all tenants which don't a tenant-specific value for this -- setting. tenant_id INT8 NOT NULL, + -- the internal key for the setting. The column is called 'name' for + -- historical reasons. name STRING NOT NULL, value STRING NOT NULL, last_updated TIMESTAMP NOT NULL DEFAULT now(), diff --git a/pkg/sql/conn_executor.go b/pkg/sql/conn_executor.go index 0f7d9e3ede73..4b55a804fcd1 100644 --- a/pkg/sql/conn_executor.go +++ b/pkg/sql/conn_executor.go @@ -889,7 +889,7 @@ func (s *Server) IncrementConnectionCount( pgerror.New(pgcode.TooManyConnections, "sorry, too many clients already"), "the maximum number of allowed connections is %d and can be modified using the %s config key", maxNumConnectionsValue, - maxNumNonAdminConnections.Key(), + maxNumNonAdminConnections.Name(), ) } return decrementConnectionCount, nil diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index 2934652cefc8..9c748f959694 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -2368,10 +2368,10 @@ CREATE TABLE crdb_internal.cluster_settings ( } for _, k := range settings.Keys(p.ExecCfg().Codec.ForSystemTenant()) { // If the user has specifically MODIFYSQLCLUSTERSETTING, hide non-sql.defaults settings. - if !hasModify && !hasView && hasSqlModify && !strings.HasPrefix(k, "sql.defaults") { + if !hasModify && !hasView && hasSqlModify && !strings.HasPrefix(string(k), "sql.defaults") { continue } - setting, _ := settings.LookupForLocalAccess(k, p.ExecCfg().Codec.ForSystemTenant()) + setting, _ := settings.LookupForLocalAccessByKey(k, p.ExecCfg().Codec.ForSystemTenant()) strVal := setting.String(&p.ExecCfg().Settings.SV) isPublic := setting.Visibility() == settings.Public desc := setting.Description() @@ -2381,7 +2381,7 @@ CREATE TABLE crdb_internal.cluster_settings ( } origin := setting.ValueOrigin(ctx, &p.ExecCfg().Settings.SV).String() if err := addRow( - tree.NewDString(k), + tree.NewDString(string(setting.Name())), tree.NewDString(strVal), tree.NewDString(setting.Typ()), tree.MakeDBool(tree.DBool(isPublic)), diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go index 56caf7715f67..111f917d7d16 100644 --- a/pkg/sql/exec_util.go +++ b/pkg/sql/exec_util.go @@ -3796,5 +3796,5 @@ func (cfg *ExecutorConfig) RequireSystemTenantOrClusterSetting( if cfg.Codec.ForSystemTenant() || setting.Get(&cfg.Settings.SV) { return nil } - return errors.Newf("tenant cluster setting %s disabled", setting.Key()) + return errors.Newf("tenant cluster setting %s disabled", setting.Name()) } diff --git a/pkg/sql/logictest/logic.go b/pkg/sql/logictest/logic.go index f67b87fb562a..d43e41656f34 100644 --- a/pkg/sql/logictest/logic.go +++ b/pkg/sql/logictest/logic.go @@ -1569,10 +1569,9 @@ func (t *logicTest) newCluster( // implicitly) set any necessary cluster settings to override blocked // behavior. if cfg.UseSecondaryTenant == logictestbase.Always || t.cluster.StartedDefaultTestTenant() { - conn := t.cluster.SystemLayer(0).SQLConn(t.rootT, "") clusterSettings := toa.clusterSettings - _, ok := clusterSettings[sql.SecondaryTenantZoneConfigsEnabled.Key()] + _, ok := clusterSettings[string(sql.SecondaryTenantZoneConfigsEnabled.Name())] if ok { // We reduce the closed timestamp duration on the host tenant so that the // setting override can propagate to the tenant faster. @@ -1589,8 +1588,8 @@ func (t *logicTest) newCluster( } tenantID := serverutils.TestTenantID() - for name, value := range clusterSettings { - query := fmt.Sprintf("ALTER TENANT [$1] SET CLUSTER SETTING %s = $2", name) + for settingName, value := range clusterSettings { + query := fmt.Sprintf("ALTER TENANT [$1] SET CLUSTER SETTING %s = $2", settingName) if _, err := conn.Exec(query, tenantID.ToUint64(), value); err != nil { t.Fatal(err) } @@ -1752,9 +1751,9 @@ func (t *logicTest) newCluster( }) } - for name, value := range toa.clusterSettings { + for settingName, value := range toa.clusterSettings { t.waitForTenantReadOnlyClusterSettingToTakeEffectOrFatal( - name, value, params.ServerArgs.Insecure, + settingName, value, params.ServerArgs.Insecure, ) } diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index b80868e816fb..ef4bbbe74e25 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -4791,7 +4791,7 @@ value if you rely on the HLC for accuracy.`, if !ok { return nil, errors.AssertionFailedf("expected string value, got %T", args[0]) } - name := strings.ToLower(string(s)) + name := settings.SettingName(strings.ToLower(string(s))) setting, ok := settings.LookupForLocalAccess(name, evalCtx.Codec.ForSystemTenant()) if !ok { return nil, errors.Newf("unknown cluster setting '%s'", name) @@ -4821,7 +4821,7 @@ value if you rely on the HLC for accuracy.`, if !ok { return nil, errors.AssertionFailedf("expected string value, got %T", args[1]) } - name := strings.ToLower(string(s)) + name := settings.SettingName(strings.ToLower(string(s))) setting, ok := settings.LookupForLocalAccess(name, evalCtx.Codec.ForSystemTenant()) if !ok { return nil, errors.Newf("unknown cluster setting '%s'", name) diff --git a/pkg/sql/sessioninit/cache.go b/pkg/sql/sessioninit/cache.go index a0b1f250c968..3dd974cdfa69 100644 --- a/pkg/sql/sessioninit/cache.go +++ b/pkg/sql/sessioninit/cache.go @@ -31,7 +31,7 @@ import ( ) // CacheEnabledSettingName is the name of the CacheEnabled cluster setting. -var CacheEnabledSettingName = "server.authentication_cache.enabled" +const CacheEnabledSettingName = "server.authentication_cache.enabled" // CacheEnabled is a cluster setting that determines if the // sessioninit.Cache and associated logic is enabled. diff --git a/pkg/sql/set_cluster_setting.go b/pkg/sql/set_cluster_setting.go index 1c05f7f826f7..d7c136cb26e0 100644 --- a/pkg/sql/set_cluster_setting.go +++ b/pkg/sql/set_cluster_setting.go @@ -58,15 +58,16 @@ import ( // setClusterSettingNode represents a SET CLUSTER SETTING statement. type setClusterSettingNode struct { - name string + name settings.SettingName st *cluster.Settings setting settings.NonMaskedSetting // If value is nil, the setting should be reset. value tree.TypedExpr } -func checkPrivilegesForSetting(ctx context.Context, p *planner, name string, action string) error { - +func checkPrivilegesForSetting( + ctx context.Context, p *planner, name settings.SettingName, action string, +) error { // First check system privileges. hasModify := false hasSqlModify := false @@ -110,7 +111,7 @@ func checkPrivilegesForSetting(ctx context.Context, p *planner, name string, act hasView = hasView || ok } - isSqlSetting := strings.HasPrefix(name, "sql.defaults") + isSqlSetting := strings.HasPrefix(string(name), "sql.defaults") // If the user has modify they can do either action to any setting regardless of // whether they have the other 2 settings. if hasModify { @@ -153,7 +154,7 @@ func checkPrivilegesForSetting(ctx context.Context, p *planner, name string, act func (p *planner) SetClusterSetting( ctx context.Context, n *tree.SetClusterSetting, ) (planNode, error) { - name := strings.ToLower(n.Name) + name := settings.SettingName(strings.ToLower(n.Name)) st := p.EvalContext().Settings setting, ok := settings.LookupForLocalAccess(name, p.ExecCfg().Codec.ForSystemTenant()) if !ok { @@ -175,7 +176,7 @@ func (p *planner) SetClusterSetting( } } - if st.OverridesInformer != nil && st.OverridesInformer.IsOverridden(name) { + if st.OverridesInformer != nil && st.OverridesInformer.IsOverridden(setting.InternalKey()) { return nil, errors.Errorf("cluster setting '%s' is currently overridden by the operator", name) } @@ -185,13 +186,16 @@ func (p *planner) SetClusterSetting( } csNode := setClusterSettingNode{ - name: name, st: st, setting: setting, value: value, + name: name, + st: st, + setting: setting, + value: value, } return &csNode, nil } func (p *planner) getAndValidateTypedClusterSetting( - ctx context.Context, name string, expr tree.Expr, setting settings.NonMaskedSetting, + ctx context.Context, name settings.SettingName, expr tree.Expr, setting settings.NonMaskedSetting, ) (tree.TypedExpr, error) { var value tree.TypedExpr if expr != nil { @@ -219,7 +223,7 @@ func (p *planner) getAndValidateTypedClusterSetting( requiredType = types.Interval // Ensure that the expression contains a unit (i.e can't be a float) _, err := p.analyzeExpr( - ctx, expr, nil, dummyHelper, types.Float, false, "SET CLUSTER SETTING "+name, + ctx, expr, nil, dummyHelper, types.Float, false, "SET CLUSTER SETTING "+string(name), ) // An interval with a unit (valid) will return an // "InvalidTextRepresentation" error when trying to parse it as a float. @@ -235,7 +239,7 @@ func (p *planner) getAndValidateTypedClusterSetting( } typed, err := p.analyzeExpr( - ctx, expr, nil, dummyHelper, requiredType, true, "SET CLUSTER SETTING "+name) + ctx, expr, nil, dummyHelper, requiredType, true, "SET CLUSTER SETTING "+string(name)) if err != nil { hasHint, hint := setting.ErrorHint() if hasHint { @@ -252,7 +256,7 @@ func (p *planner) getAndValidateTypedClusterSetting( } func (n *setClusterSettingNode) startExec(params runParams) error { - if strings.HasPrefix(n.name, "sql.defaults") { + if strings.HasPrefix(string(n.setting.InternalKey()), "sql.defaults") { params.p.BufferClientNotice( params.ctx, errors.WithHintf( @@ -366,7 +370,7 @@ func writeSettingInternal( hook VersionUpgradeHook, db isql.DB, setting settings.NonMaskedSetting, - name string, + name settings.SettingName, user username.SQLUsername, st *cluster.Settings, value tree.TypedExpr, @@ -380,7 +384,7 @@ func writeSettingInternal( if value == nil { // This code is doing work for RESET CLUSTER SETTING. var err error - reportedValue, expectedEncodedValue, err = writeDefaultSettingValue(ctx, db, setting, name) + reportedValue, expectedEncodedValue, err = writeDefaultSettingValue(ctx, db, setting) if err != nil { return err } @@ -393,7 +397,7 @@ func writeSettingInternal( } reportedValue, expectedEncodedValue, err = writeNonDefaultSettingValue( ctx, hook, db, - setting, name, user, st, value, forSystemTenant, + setting, user, st, value, forSystemTenant, releaseLeases, ) if err != nil { @@ -403,7 +407,7 @@ func writeSettingInternal( return logFn(ctx, 0, /* no target */ &eventpb.SetClusterSetting{ - SettingName: name, + SettingName: string(name), Value: reportedValue, }) }(); err != nil { @@ -417,14 +421,14 @@ func writeSettingInternal( // RESET CLUSTER SETTING statement or changing the value of a setting // to DEFAULT. func writeDefaultSettingValue( - ctx context.Context, db isql.DB, setting settings.NonMaskedSetting, name string, + ctx context.Context, db isql.DB, setting settings.NonMaskedSetting, ) (reportedValue string, expectedEncodedValue string, err error) { reportedValue = "DEFAULT" expectedEncodedValue = setting.EncodedDefault() _, err = db.Executor().ExecEx( ctx, "reset-setting", nil, sessiondata.RootUserSessionDataOverride, - "DELETE FROM system.settings WHERE name = $1", name, + "DELETE FROM system.settings WHERE name = $1", setting.InternalKey(), ) return reportedValue, expectedEncodedValue, err } @@ -436,7 +440,6 @@ func writeNonDefaultSettingValue( hook VersionUpgradeHook, db isql.DB, setting settings.NonMaskedSetting, - name string, user username.SQLUsername, st *cluster.Settings, value tree.Datum, @@ -447,7 +450,7 @@ func writeNonDefaultSettingValue( reportedValue = tree.AsStringWithFlags(value, tree.FmtBareStrings) // Validate the input and convert it to the binary encoding. - encoded, err := toSettingString(ctx, st, name, setting, value) + encoded, err := toSettingString(ctx, st, setting, value) expectedEncodedValue = encoded if err != nil { return reportedValue, expectedEncodedValue, err @@ -456,7 +459,7 @@ func writeNonDefaultSettingValue( verSetting, isSetVersion := setting.(*settings.VersionSetting) if isSetVersion { if err := setVersionSetting( - ctx, hook, verSetting, name, db, user, st, value, encoded, + ctx, hook, verSetting, db, user, st, value, encoded, forSystemTenant, releaseLeases, ); err != nil { return reportedValue, expectedEncodedValue, err @@ -467,7 +470,7 @@ func writeNonDefaultSettingValue( ctx, "update-setting", nil, sessiondata.RootUserSessionDataOverride, `UPSERT INTO system.settings (name, value, "lastUpdated", "valueType") VALUES ($1, $2, now(), $3)`, - name, encoded, setting.Typ(), + setting.InternalKey(), encoded, setting.Typ(), ); err != nil { return reportedValue, expectedEncodedValue, err } @@ -482,7 +485,6 @@ func setVersionSetting( ctx context.Context, hook VersionUpgradeHook, setting *settings.VersionSetting, - name string, db isql.DB, user username.SQLUsername, st *cluster.Settings, @@ -497,7 +499,7 @@ func setVersionSetting( datums, err := db.Executor().QueryRowEx( ctx, "retrieve-prev-setting", nil, sessiondata.RootUserSessionDataOverride, - "SELECT value FROM system.settings WHERE name = $1", name, + "SELECT value FROM system.settings WHERE name = $1", setting.InternalKey(), ) if err != nil { return err @@ -551,7 +553,7 @@ func setVersionSetting( datums, err := txn.QueryRowEx( ctx, "retrieve-prev-setting", txn.KV(), sessiondata.RootUserSessionDataOverride, - "SELECT value FROM system.settings WHERE name = $1", name, + "SELECT value FROM system.settings WHERE name = $1", setting.InternalKey(), ) if err != nil { return err @@ -580,7 +582,7 @@ func setVersionSetting( ctx, "update-setting", txn.KV(), sessiondata.RootUserSessionDataOverride, `UPSERT INTO system.settings (name, value, "lastUpdated", "valueType") VALUES ($1, $2, now(), $3)`, - name, string(rawValue), setting.Typ(), + setting.InternalKey(), string(rawValue), setting.Typ(), ); err != nil { return err } @@ -617,7 +619,7 @@ func waitForSettingUpdate( execCfg *ExecutorConfig, setting settings.NonMaskedSetting, reset bool, - name string, + name settings.SettingName, expectedEncodedValue string, ) error { if _, ok := setting.(*settings.VersionSetting); ok && reset { @@ -687,7 +689,7 @@ func (n *setClusterSettingNode) Close(_ context.Context) {} // // current value of the setting, read from the system.settings table. func toSettingString( - ctx context.Context, st *cluster.Settings, name string, s settings.Setting, d tree.Datum, + ctx context.Context, st *cluster.Settings, s settings.Setting, d tree.Datum, ) (string, error) { switch setting := s.(type) { case *settings.StringSetting: diff --git a/pkg/sql/set_var.go b/pkg/sql/set_var.go index cb1e336f293b..6b9b85723a89 100644 --- a/pkg/sql/set_var.go +++ b/pkg/sql/set_var.go @@ -65,7 +65,7 @@ func (p *planner) SetVar(ctx context.Context, n *tree.SetVar) (planNode, error) if err != nil { return nil, err } - if _, ok := settings.LookupForLocalAccess(name, p.ExecCfg().Codec.ForSystemTenant()); ok { + if _, ok := settings.LookupForLocalAccess(settings.SettingName(name), p.ExecCfg().Codec.ForSystemTenant()); ok { p.BufferClientNotice( ctx, errors.WithHint( diff --git a/pkg/sql/show_cluster_setting.go b/pkg/sql/show_cluster_setting.go index 409f821efd7d..97660daa8eef 100644 --- a/pkg/sql/show_cluster_setting.go +++ b/pkg/sql/show_cluster_setting.go @@ -39,7 +39,10 @@ import ( // the version setting. The caller is responsible for decoding the // value to transform it to a user-facing string. func (p *planner) getCurrentEncodedVersionSettingValue( - ctx context.Context, s *settings.VersionSetting, name string, + ctx context.Context, + s *settings.VersionSetting, + key settings.InternalKey, + name settings.SettingName, ) (string, error) { st := p.ExecCfg().Settings var res string @@ -61,7 +64,7 @@ func (p *planner) getCurrentEncodedVersionSettingValue( ctx, "read-setting", txn.KV(), sessiondata.RootUserSessionDataOverride, - "SELECT value FROM system.settings WHERE name = $1", name, + "SELECT value FROM system.settings WHERE name = $1", key, ) if err != nil { return err @@ -152,7 +155,7 @@ func checkClusterSettingValuesAreEquivalent(localRawVal, kvRawVal []byte) error func (p *planner) ShowClusterSetting( ctx context.Context, n *tree.ShowClusterSetting, ) (planNode, error) { - name := strings.ToLower(n.Name) + name := settings.SettingName(strings.ToLower(n.Name)) setting, ok := settings.LookupForLocalAccess(name, p.ExecCfg().Codec.ForSystemTenant()) if !ok { return nil, errors.Errorf("unknown setting: %q", name) @@ -166,7 +169,7 @@ func (p *planner) ShowClusterSetting( p.BufferClientNotice( ctx, errors.WithHintf( - pgnotice.Newf("using global default %s is not recommended", n.Name), + pgnotice.Newf("using global default %s is not recommended", name), "use the `ALTER ROLE ... SET` syntax to control session variable defaults at a finer-grained level. See: %s", docs.URL("alter-role.html#set-default-session-variable-values-for-a-role"), ), @@ -181,7 +184,7 @@ func (p *planner) ShowClusterSetting( return planShowClusterSetting(setting, name, columns, func(ctx context.Context, p *planner) (bool, string, error) { if verSetting, ok := setting.(*settings.VersionSetting); ok { - encoded, err := p.getCurrentEncodedVersionSettingValue(ctx, verSetting, name) + encoded, err := p.getCurrentEncodedVersionSettingValue(ctx, verSetting, setting.InternalKey(), name) return true, encoded, err } return true, setting.Encoded(&p.ExecCfg().Settings.SV), nil @@ -190,7 +193,7 @@ func (p *planner) ShowClusterSetting( } func getShowClusterSettingPlanColumns( - val settings.NonMaskedSetting, name string, + val settings.NonMaskedSetting, name settings.SettingName, ) (colinfo.ResultColumns, error) { var dType *types.T switch val.(type) { @@ -209,17 +212,17 @@ func getShowClusterSettingPlanColumns( default: return nil, errors.Errorf("unknown setting type for %s: %s", name, val.Typ()) } - return colinfo.ResultColumns{{Name: name, Typ: dType}}, nil + return colinfo.ResultColumns{{Name: string(name), Typ: dType}}, nil } func planShowClusterSetting( val settings.NonMaskedSetting, - name string, + name settings.SettingName, columns colinfo.ResultColumns, getEncodedValue func(ctx context.Context, p *planner) (bool, string, error), ) (planNode, error) { return &delayedNode{ - name: "SHOW CLUSTER SETTING " + name, + name: "SHOW CLUSTER SETTING " + string(name), columns: columns, constructor: func(ctx context.Context, p *planner) (planNode, error) { isNotNull, encoded, err := getEncodedValue(ctx, p) diff --git a/pkg/sql/show_test.go b/pkg/sql/show_test.go index 1659641c0a57..8bfae93bcd5e 100644 --- a/pkg/sql/show_test.go +++ b/pkg/sql/show_test.go @@ -1214,13 +1214,13 @@ func TestLintClusterSettingNames(t *testing.T) { defer rows.Close() for rows.Next() { - var varName, sType, desc string - if err := rows.Scan(&varName, &sType, &desc); err != nil { + var settingKey, sType, desc string + if err := rows.Scan(&settingKey, &sType, &desc); err != nil { t.Fatal(err) } - if strings.ToLower(varName) != varName { - t.Errorf("%s: variable name must be all lowercase", varName) + if strings.ToLower(settingKey) != settingKey { + t.Errorf("%s: variable name must be all lowercase", settingKey) } suffixSuggestions := map[string]string{ @@ -1230,37 +1230,37 @@ func TestLintClusterSettingNames(t *testing.T) { } nameErr := func() error { - segments := strings.Split(varName, ".") + segments := strings.Split(settingKey, ".") for _, segment := range segments { if strings.TrimSpace(segment) != segment { - return errors.Errorf("%s: part %q has heading or trailing whitespace", varName, segment) + return errors.Errorf("%s: part %q has heading or trailing whitespace", settingKey, segment) } tokens, ok := parser.Tokens(segment) if !ok { - return errors.Errorf("%s: part %q does not scan properly", varName, segment) + return errors.Errorf("%s: part %q does not scan properly", settingKey, segment) } if len(tokens) == 0 || len(tokens) > 1 { - return errors.Errorf("%s: part %q has invalid structure", varName, segment) + return errors.Errorf("%s: part %q has invalid structure", settingKey, segment) } if tokens[0].TokenID != parser.IDENT { cat, ok := lexbase.KeywordsCategories[tokens[0].Str] if !ok { - return errors.Errorf("%s: part %q has invalid structure", varName, segment) + return errors.Errorf("%s: part %q has invalid structure", settingKey, segment) } if cat == "R" { - return errors.Errorf("%s: part %q is a reserved keyword", varName, segment) + return errors.Errorf("%s: part %q is a reserved keyword", settingKey, segment) } } } for suffix, repl := range suffixSuggestions { - if strings.HasSuffix(varName, suffix) { - return errors.Errorf("%s: use %q instead of %q", varName, repl, suffix) + if strings.HasSuffix(settingKey, suffix) { + return errors.Errorf("%s: use %q instead of %q", settingKey, repl, suffix) } } - if sType == "b" && !strings.HasSuffix(varName, ".enabled") { - return errors.Errorf("%s: use .enabled for booleans", varName) + if sType == "b" && !strings.HasSuffix(settingKey, ".enabled") { + return errors.Errorf("%s: use .enabled for booleans", settingKey) } return nil @@ -1311,27 +1311,27 @@ func TestLintClusterSettingNames(t *testing.T) { "sql.defaults.idle_in_transaction_session_timeout": `sql.defaults.idle_in_transaction_session_timeout: use ".timeout" instead of "_timeout"`, "cloudstorage.gs.chunking.retry_timeout": `cloudstorage.gs.chunking.retry_timeout: use ".timeout" instead of "_timeout"`, } - expectedErr, found := grandFathered[varName] + expectedErr, found := grandFathered[settingKey] if !found || expectedErr != nameErr.Error() { t.Error(nameErr) } } if strings.TrimSpace(desc) != desc { - t.Errorf("%s: description %q has heading or trailing whitespace", varName, desc) + t.Errorf("%s: description %q has heading or trailing whitespace", settingKey, desc) } if len(desc) == 0 { - t.Errorf("%s: description is empty", varName) + t.Errorf("%s: description is empty", settingKey) } if len(desc) > 0 { if strings.ToLower(desc[0:1]) != desc[0:1] { - t.Errorf("%s: description %q must not start with capital", varName, desc) + t.Errorf("%s: description %q must not start with capital", settingKey, desc) } if sType != "e" && (desc[len(desc)-1] == '.') && !strings.Contains(desc, ". ") { // TODO(knz): this check doesn't work with the way enum values are added to their descriptions. - t.Errorf("%s: description %q must end with period only if it contains a secondary sentence", varName, desc) + t.Errorf("%s: description %q must end with period only if it contains a secondary sentence", settingKey, desc) } } } diff --git a/pkg/sql/sqlstats/sslocal/sql_stats_test.go b/pkg/sql/sqlstats/sslocal/sql_stats_test.go index 6065dbdd2f95..3385587dcea3 100644 --- a/pkg/sql/sqlstats/sslocal/sql_stats_test.go +++ b/pkg/sql/sqlstats/sslocal/sql_stats_test.go @@ -696,7 +696,7 @@ func TestAssociatingStmtStatsWithTxnFingerprint(t *testing.T) { testutils.RunTrueAndFalse(t, "enabled", func(t *testing.T, enabled bool) { // Establish the cluster setting. setting := sslocal.AssociateStmtWithTxnFingerprint - err := updater.Set(ctx, setting.Key(), settings.EncodedValue{ + err := updater.Set(ctx, setting.InternalKey(), settings.EncodedValue{ Value: settings.EncodeBool(enabled), Type: setting.Typ(), }) diff --git a/pkg/sql/tenant_settings.go b/pkg/sql/tenant_settings.go index 268c4f85d4e5..0bf8db18a30c 100644 --- a/pkg/sql/tenant_settings.go +++ b/pkg/sql/tenant_settings.go @@ -29,7 +29,7 @@ import ( // alterTenantSetClusterSettingNode represents an // ALTER VIRTUAL CLUSTER ... SET CLUSTER SETTING statement. type alterTenantSetClusterSettingNode struct { - name string + name settings.SettingName tenantSpec tenantSpec st *cluster.Settings setting settings.NonMaskedSetting @@ -55,7 +55,7 @@ func (p *planner) AlterTenantSetClusterSetting( "ALTER VIRTUAL CLUSTER can only be called by system operators") } - name := strings.ToLower(n.Name) + name := settings.SettingName(strings.ToLower(n.Name)) st := p.EvalContext().Settings setting, ok := settings.LookupForLocalAccess(name, true /* forSystemTenant - checked above already */) if !ok { @@ -67,7 +67,7 @@ func (p *planner) AlterTenantSetClusterSetting( "%s is a system-only setting and must be set in the admin tenant using SET CLUSTER SETTING", name) } - tspec, err := p.planTenantSpec(ctx, n.TenantSpec, "ALTER VIRTUAL CLUSTER SET CLUSTER SETTING "+name) + tspec, err := p.planTenantSpec(ctx, n.TenantSpec, "ALTER VIRTUAL CLUSTER SET CLUSTER SETTING "+string(name)) if err != nil { return nil, err } @@ -127,7 +127,7 @@ func (n *alterTenantSetClusterSettingNode) startExec(params runParams) error { if err != nil { return err } - encoded, err := toSettingString(params.ctx, n.st, n.name, n.setting, value) + encoded, err := toSettingString(params.ctx, n.st, n.setting, value) if err != nil { return err } @@ -146,7 +146,7 @@ func (n *alterTenantSetClusterSettingNode) startExec(params runParams) error { params.ctx, 0, /* no target */ &eventpb.SetTenantClusterSetting{ - SettingName: n.name, + SettingName: string(n.name), Value: reportedValue, TenantId: tenantID, AllTenants: tenantID == 0, @@ -173,7 +173,7 @@ func (p *planner) ShowTenantClusterSetting( return nil, err } - name := strings.ToLower(n.Name) + name := settings.SettingName(strings.ToLower(n.Name)) setting, ok := settings.LookupForLocalAccess(name, p.ExecCfg().Codec.ForSystemTenant()) if !ok { return nil, errors.Errorf("unknown setting: %q", name) @@ -236,7 +236,7 @@ FROM ctx, "get-tenant-setting-value", p.txn, sessiondata.RootUserSessionDataOverride, lookupEncodedTenantSetting, - name, rec.ID) + setting.InternalKey(), rec.ID) if err != nil { return false, "", err } diff --git a/pkg/sql/ttl/ttljob/ttljob.go b/pkg/sql/ttl/ttljob/ttljob.go index 9a2ea6c78c32..8153940a77f9 100644 --- a/pkg/sql/ttl/ttljob/ttljob.go +++ b/pkg/sql/ttl/ttljob/ttljob.go @@ -330,7 +330,7 @@ func checkEnabled(settingsValues *settings.Values) error { if enabled := jobEnabled.Get(settingsValues); !enabled { return errors.Newf( "ttl jobs are currently disabled by CLUSTER SETTING %s", - jobEnabled.Key(), + jobEnabled.Name(), ) } return nil diff --git a/pkg/testutils/lint/passes/redactcheck/redactcheck.go b/pkg/testutils/lint/passes/redactcheck/redactcheck.go index 0881ff9168ec..b8ffdd084ebc 100644 --- a/pkg/testutils/lint/passes/redactcheck/redactcheck.go +++ b/pkg/testutils/lint/passes/redactcheck/redactcheck.go @@ -60,6 +60,11 @@ func runAnalyzer(pass *analysis.Pass) (interface{}, error) { "StoreIDContainer": {}, "SQLInstanceID": {}, }, + "github.com/cockroachdb/cockroach/pkg/settings": { + "InternalKey": {}, + "SettingName": {}, + "ValueOrigin": {}, + }, "github.com/cockroachdb/cockroach/pkg/cli/exit": { "Code": {}, }, diff --git a/pkg/upgrade/upgrades/BUILD.bazel b/pkg/upgrade/upgrades/BUILD.bazel index 3acbb2b1614f..4e96e12f665e 100644 --- a/pkg/upgrade/upgrades/BUILD.bazel +++ b/pkg/upgrade/upgrades/BUILD.bazel @@ -52,6 +52,7 @@ go_library( "//pkg/multitenant/mtinfopb", "//pkg/roachpb", "//pkg/security/username", + "//pkg/settings", "//pkg/settings/cluster", "//pkg/sql/catalog", "//pkg/sql/catalog/catalogkeys", diff --git a/pkg/upgrade/upgrades/ensure_sql_schema_telemetry_schedule_test.go b/pkg/upgrade/upgrades/ensure_sql_schema_telemetry_schedule_test.go index 52a8652adf2e..2571b7e175bb 100644 --- a/pkg/upgrade/upgrades/ensure_sql_schema_telemetry_schedule_test.go +++ b/pkg/upgrade/upgrades/ensure_sql_schema_telemetry_schedule_test.go @@ -96,11 +96,11 @@ func TestSchemaTelemetrySchedule(t *testing.T) { // Check that the schedule can have its recurrence altered. tdb.Exec(t, fmt.Sprintf(`SET CLUSTER SETTING %s = '* * * * *'`, - schematelemetrycontroller.SchemaTelemetryRecurrence.Key())) + schematelemetrycontroller.SchemaTelemetryRecurrence.InternalKey())) tdb.CheckQueryResultsRetry(t, qExists, [][]string{{"* * * * *", "1"}}) exp = scheduledjobs.MaybeRewriteCronExpr(clusterID, "@daily") tdb.Exec(t, fmt.Sprintf(`SET CLUSTER SETTING %s = '@daily'`, - schematelemetrycontroller.SchemaTelemetryRecurrence.Key())) + schematelemetrycontroller.SchemaTelemetryRecurrence.InternalKey())) tdb.CheckQueryResultsRetry(t, qExists, [][]string{{exp, "1"}}) }) diff --git a/pkg/upgrade/upgrades/upgrades.go b/pkg/upgrade/upgrades/upgrades.go index 3dc47e68cfc8..51d39e8d505b 100644 --- a/pkg/upgrade/upgrades/upgrades.go +++ b/pkg/upgrade/upgrades/upgrades.go @@ -19,6 +19,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/upgrade" "github.com/cockroachdb/cockroach/pkg/upgrade/upgradebase" ) @@ -26,7 +27,7 @@ import ( // SettingsDefaultOverrides documents the effect of several migrations that add // an explicit value for a setting, effectively changing the "default value" // from what was defined in code. -var SettingsDefaultOverrides = map[string]string{ +var SettingsDefaultOverrides = map[settings.InternalKey]string{ "diagnostics.reporting.enabled": "true", "cluster.secret": "", }