Merge #109048

109048: settings: allow setting names to diverge from the key r=stevendanna a=knz

Informs #109046.
Epic: CRDB-28893

This patch makes it possible to change the name of cluster settings
over time while preserving backward-compatibility.

For example:
```go
var s = RegisterDurationSetting(
  "server.web_session_timeout", // <- this is the key; it is immutable.
  settings.WithName("server.web_session.timeout"), // <- user-visible name
)
```

When the name is not specified, the key is used as name. If the name
is specified, it must not exist already - neither as a key, nor as a
name for another setting, nor as a previously-known name.

If later, it becomes necessary to change the name again, the
`WithRetiredName` option can be used. This is necessary to properly
redirect users to the new name.

For example:
```go
var s = RegisterDurationSetting(
  "server.web_session_timeout", // <- this is the key; it is immutable.
  settings.WithName("http.web_session.timeout"), // <- new user-visible name
  settings.WithRetiredName("server.web_session.timeout"), // <- past name
)
```

A pgwire notice is sent to the user if they attempt to use SHOW/SET
CLUSTER SETTING using a retired name.

Release note: None

Co-authored-by: Raphael 'kena' Poss <[email protected]>

pkg/server/admin.go

@@ -2020,7 +2020,7 @@ func (s *adminServer) Settings(
 	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))
+		key, ok, _ := settings.NameToKey(settings.SettingName(desiredSetting))
 		if ok {
 			settingsKeys = append(settingsKeys, key)
 		} else {

pkg/server/settingswatcher/settings_watcher_external_test.go

@@ -301,7 +301,7 @@ func TestSettingsWatcherWithOverrides(t *testing.T) {
 	expectSoon("i1", "10")
 
 	// Verify that version cannot be overridden.
-	version, ok := settings.LookupForLocalAccess("version", settings.ForSystemTenant)
+	version, ok, _ := settings.LookupForLocalAccess("version", settings.ForSystemTenant)
 	require.True(t, ok)
 	versionValue := version.String(&st.SV)
 

pkg/settings/BUILD.bazel

@@ -45,6 +45,7 @@ go_test(
     name = "settings_test",
     size = "small",
     srcs = [
+        "alias_test.go",
         "encoding_test.go",
         "internal_test.go",
         "settings_test.go",
@@ -59,6 +60,7 @@ go_test(
         "//pkg/util/protoutil",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_cockroachdb_redact//:redact",
+        "@com_github_stretchr_testify//assert",
         "@com_github_stretchr_testify//require",
     ],
 )

pkg/settings/alias_test.go

@@ -0,0 +1,89 @@
+// Copyright 2023 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package settings_test
+
+import (
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/settings"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestAliasedSettings(t *testing.T) {
+	defer settings.TestingSaveRegistry()()
+
+	_ = settings.RegisterBoolSetting(settings.SystemOnly, "s1key", "desc", false)
+	_ = settings.RegisterBoolSetting(settings.SystemOnly, "s2key", "desc", false, settings.WithName("s2name"))
+	_ = settings.RegisterBoolSetting(settings.SystemOnly, "s3key", "desc", false,
+		settings.WithName("s3name-new"), settings.WithRetiredName("s3name-old"))
+
+	k, found, _ := settings.NameToKey("unknown")
+	assert.False(t, found)
+	assert.Equal(t, settings.InternalKey(""), k)
+
+	k, found, status := settings.NameToKey("s1key")
+	assert.True(t, found)
+	assert.Equal(t, settings.InternalKey("s1key"), k)
+	assert.Equal(t, settings.NameActive, status)
+
+	k, found, status = settings.NameToKey("s2key")
+	assert.True(t, found)
+	assert.Equal(t, settings.InternalKey("s2key"), k)
+	assert.Equal(t, settings.NameRetired, status)
+
+	k, found, status = settings.NameToKey("s2name")
+	assert.True(t, found)
+	assert.Equal(t, settings.InternalKey("s2key"), k)
+	assert.Equal(t, settings.NameActive, status)
+
+	k, found, status = settings.NameToKey("s3key")
+	assert.True(t, found)
+	assert.Equal(t, settings.InternalKey("s3key"), k)
+	assert.Equal(t, settings.NameRetired, status)
+
+	k, found, status = settings.NameToKey("s3name-new")
+	assert.True(t, found)
+	assert.Equal(t, settings.InternalKey("s3key"), k)
+	assert.Equal(t, settings.NameActive, status)
+
+	k, found, status = settings.NameToKey("s3name-old")
+	assert.True(t, found)
+	assert.Equal(t, settings.InternalKey("s3key"), k)
+	assert.Equal(t, settings.NameRetired, status)
+
+	s, found, status := settings.LookupForLocalAccess("s1key", true)
+	assert.True(t, found)
+	assert.Equal(t, settings.InternalKey("s1key"), s.InternalKey())
+	assert.Equal(t, settings.NameActive, status)
+
+	s, found, status = settings.LookupForLocalAccess("s2key", true)
+	assert.True(t, found)
+	assert.Equal(t, settings.InternalKey("s2key"), s.InternalKey())
+	assert.Equal(t, settings.SettingName("s2name"), s.Name())
+	assert.Equal(t, settings.NameRetired, status)
+
+	s, found, status = settings.LookupForLocalAccess("s2name", true)
+	assert.True(t, found)
+	assert.Equal(t, settings.InternalKey("s2key"), s.InternalKey())
+	assert.Equal(t, settings.NameActive, status)
+
+	s, found, status = settings.LookupForLocalAccess("s3key", true)
+	assert.True(t, found)
+	assert.Equal(t, settings.InternalKey("s3key"), s.InternalKey())
+	assert.Equal(t, settings.SettingName("s3name-new"), s.Name())
+	assert.Equal(t, settings.NameRetired, status)
+
+	s, found, status = settings.LookupForLocalAccess("s3name-old", true)
+	assert.True(t, found)
+	assert.Equal(t, settings.InternalKey("s3key"), s.InternalKey())
+	assert.Equal(t, settings.SettingName("s3name-new"), s.Name())
+	assert.Equal(t, settings.NameRetired, status)
+}

pkg/settings/common.go

@@ -13,12 +13,15 @@ package settings
 import (
 	"context"
 	"fmt"
+
+	"github.com/cockroachdb/errors"
 )
 
 // common implements basic functionality used by all setting types.
 type common struct {
 	class         Class
 	key           InternalKey
+	name          SettingName
 	description   string
 	visibility    Visibility
 	slot          slotIdx
@@ -36,6 +39,7 @@ type slotIdx int32
 func (c *common) init(class Class, key InternalKey, description string, slot slotIdx) {
 	c.class = class
 	c.key = key
+	c.name = SettingName(key) // until overridden
 	c.description = description
 	if slot < 0 {
 		panic(fmt.Sprintf("Invalid slot index %d", slot))
@@ -55,7 +59,7 @@ func (c common) InternalKey() InternalKey {
 }
 
 func (c common) Name() SettingName {
-	return SettingName(c.key)
+	return c.name
 }
 
 func (c common) Description() string {
@@ -106,6 +110,15 @@ func (c *common) setRetired() {
 	c.retired = true
 }
 
+// setName is used to override the name of the setting.
+// Refer to the WithName option for details.
+func (c *common) setName(name SettingName) {
+	if c.name != SettingName(c.key) {
+		panic(errors.AssertionFailedf("duplicate use of WithName"))
+	}
+	c.name = name
+}
+
 // SetOnChange installs a callback to be called when a setting's value changes.
 // `fn` should avoid doing long-running or blocking work as it is called on the
 // goroutine which handles all settings updates.

pkg/settings/doc.go

@@ -12,33 +12,55 @@
 Package settings provides a central registry of runtime editable settings and
 accompanying helper functions for retrieving their current values.
 
-Settings values are stored in the system.settings table (which is gossiped). A
-gossip-driven worker updates this package's cached value when the table changes
-(see the `RefreshSettings` worker in the `sql` package).
+# Overview
 
-The package's cache is global -- while all the usual drawbacks of mutable global
-state obviously apply, it is needed to make the package's functionality
-available to a wide variety of callsites, that may or may not have a *Server or
-similar available to access settings.
+Settings values are stored in the system.settings table. A
+rangefeed-driven worker updates the cached value when the table
+changes (see package 'settingswatcher').
 
 To add a new setting, call one of the `Register` methods in `registry.go` and
 save the accessor created by the register function in the package where the
 setting is to be used. For example, to add an "enterprise" flag, adding into
 license_check.go:
 
-var enterpriseEnabled = settings.RegisterBoolSetting(
+	var enterpriseEnabled = settings.RegisterBoolSetting(
+		settings.TenantWritable,
+		"enterprise.enabled",
+		"some doc for the setting",
+		false,
+	)
 
-	settings.TenantWritable,
-	"enterprise.enabled",
-	"some doc for the setting",
-	false,
+Then use with `if enterpriseEnabled.Get(...) ...`
 
-)
+# Setting names
 
-Then use with `if enterpriseEnabled.Get() ...`
+Settings have both a "key" and a "name".
+
+The key is what is used to persist the value and synchronize it across
+nodes. The key should remain immutable through the lifetime of the
+setting. This is the main argument passed through the Register() calls.
+
+The name is what end-users see in docs and can use via the SQL
+statements SET/SHOW CLUSTER SETTING. It can change over time. It is
+also subject to a linter; for example boolean settings should have a
+name ending with '.enabled'.
+
+When no name is specified, it defaults to the key. Another name
+can be specified using `WithName()`, for example:
+
+	var mySetting = settings.RegisterBoolSetting(
+		"mykey", ...,
+		settings.WithName("descriptive.name.enabled"),
+	)
+
+For convenience, users can also refer to a setting using its key
+in the SET/SHOW CLUSTER SETTING statements. Because of this,
+the keys and names are in the same namespace and cannot overlap.
+
+# Careful choice of default values, value propagation delay
 
 Settings should always be defined with "safe" default values -- until a node
-receives values via gossip, or even after that, if it cannot read them for some
+receives values asynchronously, or even after that, if it cannot read them for some
 reason, it will use the default values, so define defaults that "fail safe".
 
 In cases where the "safe" default doesn't actually match the desired default,
@@ -52,22 +74,42 @@ rate limit into a client or something, passing a `*FooSetting` rather than a
 `Foo` and waiting to call `.Get()` until the value is actually used ensures
 observing the latest value.
 
+# Changing names of settings
+
+Sometimes for UX or documentation purposes it is useful to rename a setting.
+However, to preserve backward-compatibility, its key cannot change.
+
+There are two situations possible:
+
+  - The setting currently has the same name as its key. To rename the setting,
+    use the `WithName()` option.
+
+  - The setting already has another name than its key. In this case,
+    modify the name in the `WithName()` option and also add a
+    `WithRetiredName()` option with the previous name. A SQL notice
+    will redirect users to the new name.
+
+# Retiring settings
+
 Settings may become irrelevant over time, especially when introduced to provide
-a workaround to a system limitation which is later corrected. When deleting a
-setting's registration from the codebase, add its name to the list of
-`retiredSettings` in settings/registry.go -- this ensures the name cannot be
-accidentally reused, and suppresses log spam about the existing value.
+a workaround to a system limitation which is later corrected. There are two
+possible scenarios:
 
-That list of retired settings can periodically (i.e. in major versions) be
+  - The setting may still be referred to from automation (e.g. external
+    scripts). In this case, use the option `settings.Retired` at the
+    registration point.
+
+  - The setting will not ever be reused anywhere and can be deleted.
+    When deleting a setting's registration from the codebase, add its
+    name to the list of `retiredSettings` in settings/registry.go --
+    this ensures the name cannot be accidentally reused, and suppresses
+    log spam about the existing value.
+
+The list of deleted+retired settings can periodically (i.e. in major versions) be
 "flushed" by adding a migration that deletes all stored values for those keys
 at which point the key would be available for reuse in a later version. Is is
 only safe to run such a migration after the cluster upgrade process ensures no
 older nodes are still using the values for those old settings though, so such a
 migration needs to be version gated.
-
-Existing/off-the-shelf systems generally will not be defined in terms of our
-settings, but if they can either be swapped at runtime or expose some `setFoo`
-method, that can be used in conjunction with a change callback registered via
-OnChange.
 */
 package settings

pkg/settings/options.go

@@ -26,6 +26,32 @@ type SettingOption struct {
 	validateProtoFn    func(*Values, protoutil.Message) error
 }
 
+// NameStatus indicates the status of a setting name.
+type NameStatus bool
+
+const (
+	// NameActive indicates that the name is currently in use.
+	NameActive NameStatus = true
+	// NameRetired indicates that the name is no longer in use.
+	NameRetired NameStatus = false
+)
+
+// WithName configures the user-visible name of the setting.
+func WithName(name SettingName) SettingOption {
+	return SettingOption{commonOpt: func(c *common) {
+		c.setName(name)
+		registerAlias(c.key, name, NameActive)
+	}}
+}
+
+// WithRetiredName configures a previous user-visible name of the setting,
+// when that name was diferent from the key and is not in use any more.
+func WithRetiredName(name SettingName) SettingOption {
+	return SettingOption{commonOpt: func(c *common) {
+		registerAlias(c.key, name, NameRetired)
+	}}
+}
+
 // WithVisibility customizes the visibility of a setting.
 func WithVisibility(v Visibility) SettingOption {
 	return SettingOption{commonOpt: func(c *common) {