settings,*: update the settings.Setting API

TLDR: this commit introduces an API distinction between:

- the *name* of a cluster setting used for user-facing UX,
  including the SQL syntax and error messages.
- the *key* of a cluster setting used to store its value
  in `system.settings` and organize various data structures.

(In this commit, the name and key remain equivalent to each other;
only the interfaces change.)

This change achieves the following:

- it introduces specific Go types for both key and name. The benefit
  here is to avoid the go `string` type; and force users of the API to
  consider what data is used through the settings API.

  As a side benefit, it ensures that every setting key or name
  included in logs and error messages is not redacted away.

- it paves the way for a later change where the name is allowed to
  diverge from the key. This will allow us to enhance UX without
  breaking compatibility.

- as a side-benefit, it also marks the "setting origin" attribute
  as non-redactable.

Salient API change, prior to this change:
```go
type Setting interface {
  // Key returns the name of the specific cluster setting.
  Key() string
}
```

After this change:

```go
type Setting interface {
  // 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.
  // This is suitable for:
  // - SHOW/SET CLUSTER SETTING.
  // - inclusion in error messages.
  Name() SettingName
}
```

Release note: None

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) |
 
 
 

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",

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.

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.

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(&currentVal)
 

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(),
 				)
 			}
 		}

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,
 			))
 		}

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

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
 		}

pkg/cli/gen.go

@@ -215,23 +215,24 @@ 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(`<div id="setting-%s" class="anchored">%s</div>`, slugify.Slugify(s), wrapCode(s))
+				return fmt.Sprintf(`<div id="setting-%s" class="anchored">%s</div>`, slugify.Slugify(string(key)), wrapCode(string(name)))
 			}
-			return s
+			return string(name)
 		}
 
 		// Fill a Values struct with the defaults.
 		s := cluster.MakeTestingClusterSettings()
 		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 = `<a href="alter-role.html"><code>ALTER ROLE... SET</code></a>`
 			}
-			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() {

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

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 {

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

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
 }

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"
 	}

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];
 }
 

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")

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.