From c71ec449ab694c1e43bb5b1549ecbceff60b7524 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Fri, 18 Aug 2023 18:45:47 +0200 Subject: [PATCH] settings: allow setting names to diverge from the key 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 --- pkg/server/admin.go | 2 +- .../settings_watcher_external_test.go | 2 +- pkg/settings/BUILD.bazel | 2 + pkg/settings/alias_test.go | 89 ++++++++++++++++++ pkg/settings/common.go | 15 ++- pkg/settings/doc.go | 92 ++++++++++++++----- pkg/settings/options.go | 26 ++++++ pkg/settings/registry.go | 80 ++++++++++++---- pkg/settings/settings_test.go | 12 +-- pkg/sql/sem/builtins/builtins.go | 4 +- pkg/sql/set_cluster_setting.go | 6 +- pkg/sql/set_var.go | 2 +- pkg/sql/show_cluster_setting.go | 10 +- pkg/sql/tenant_settings.go | 12 ++- 14 files changed, 296 insertions(+), 58 deletions(-) create mode 100644 pkg/settings/alias_test.go diff --git a/pkg/server/admin.go b/pkg/server/admin.go index cbf19145b289..6dfef9076122 100644 --- a/pkg/server/admin.go +++ b/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 { diff --git a/pkg/server/settingswatcher/settings_watcher_external_test.go b/pkg/server/settingswatcher/settings_watcher_external_test.go index b9bdc54936e2..f1739f49d519 100644 --- a/pkg/server/settingswatcher/settings_watcher_external_test.go +++ b/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) diff --git a/pkg/settings/BUILD.bazel b/pkg/settings/BUILD.bazel index f349263b87ab..51dac52ee91e 100644 --- a/pkg/settings/BUILD.bazel +++ b/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", ], ) diff --git a/pkg/settings/alias_test.go b/pkg/settings/alias_test.go new file mode 100644 index 000000000000..d53c9d0fad42 --- /dev/null +++ b/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) +} diff --git a/pkg/settings/common.go b/pkg/settings/common.go index 12b464f793b5..3a040da53e29 100644 --- a/pkg/settings/common.go +++ b/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. diff --git a/pkg/settings/doc.go b/pkg/settings/doc.go index 33b39f8a9f29..a4cf1414aa3f 100644 --- a/pkg/settings/doc.go +++ b/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 diff --git a/pkg/settings/options.go b/pkg/settings/options.go index 1b793d1303ef..81ae40ecae5d 100644 --- a/pkg/settings/options.go +++ b/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) { diff --git a/pkg/settings/registry.go b/pkg/settings/registry.go index e6a1c4b35c5c..ba45587b79c7 100644 --- a/pkg/settings/registry.go +++ b/pkg/settings/registry.go @@ -28,6 +28,17 @@ import ( // read concurrently by different callers. var registry = make(map[InternalKey]internalSetting) +// aliasRegistry contains the mapping of names to keys, for names +// different from the keys. +var aliasRegistry = make(map[SettingName]aliasEntry) + +type aliasEntry struct { + // key is the setting this name is referring to. + key InternalKey + // active indicates whether this name is currently in use. + active NameStatus +} + // slotTable stores the same settings as the registry, but accessible by the // slot index. var slotTable [MaxSettings]internalSetting @@ -39,8 +50,13 @@ func TestingSaveRegistry() func() { for k, v := range registry { origRegistry[k] = v } + var origAliases = make(map[SettingName]aliasEntry) + for k, v := range aliasRegistry { + origAliases[k] = v + } return func() { registry = origRegistry + aliasRegistry = origAliases } } @@ -232,21 +248,29 @@ var sqlDefaultSettings = map[InternalKey]struct{}{ "sql.defaults.zigzag_join.enabled": {}, } -// register adds a setting to the registry. -func register(class Class, key InternalKey, desc string, s internalSetting) { - if _, ok := retiredSettings[key]; ok { - panic(fmt.Sprintf("cannot reuse previously defined setting key: %s", key)) +// checkNameFound verifies whether the given string is known as key or name. +func checkNameFound(keyOrName string) { + if _, ok := retiredSettings[InternalKey(keyOrName)]; ok { + panic(fmt.Sprintf("cannot reuse previously defined setting key: %s", InternalKey(keyOrName))) + } + if _, ok := registry[InternalKey(keyOrName)]; ok { + panic(fmt.Sprintf("setting already defined: %s", keyOrName)) } - if _, ok := registry[key]; ok { - panic(fmt.Sprintf("setting already defined: %s", key)) + if a, ok := aliasRegistry[SettingName(keyOrName)]; ok { + panic(fmt.Sprintf("setting already defined: %s (with key %s)", keyOrName, a.key)) } - if strings.Contains(string(key), "sql.defaults") { - if _, ok := sqlDefaultSettings[key]; !ok { + if strings.Contains(keyOrName, "sql.defaults") { + if _, ok := sqlDefaultSettings[InternalKey(keyOrName)]; !ok { panic(fmt.Sprintf( "new sql.defaults cluster settings: %s is not needed now that `ALTER ROLE ... SET` syntax "+ - "is supported; please remove the new sql.defaults cluster setting", key)) + "is supported; please remove the new sql.defaults cluster setting", keyOrName)) } } +} + +// register adds a setting to the registry. +func register(class Class, key InternalKey, desc string, s internalSetting) { + checkNameFound(string(key)) if len(desc) == 0 { panic(fmt.Sprintf("setting missing description: %s", key)) } @@ -271,6 +295,11 @@ func register(class Class, key InternalKey, desc string, s internalSetting) { slotTable[slot] = s } +func registerAlias(key InternalKey, name SettingName, nameStatus NameStatus) { + checkNameFound(string(name)) + aliasRegistry[name] = aliasEntry{key: key, active: nameStatus} +} + // NumRegisteredSettings returns the number of registered settings. func NumRegisteredSettings() int { return len(registry) } @@ -316,19 +345,36 @@ var allConsoleKeys = []InternalKey{ } // NameToKey returns the key associated with a setting name. -func NameToKey(name SettingName) (InternalKey, bool) { - // TODO(knz): improve this. - key := InternalKey(name) - return key, true +func NameToKey(name SettingName) (key InternalKey, found bool, nameStatus NameStatus) { + // First check the alias registry. + if alias, ok := aliasRegistry[name]; ok { + return alias.key, true, alias.active + } + // No alias: did they perhaps use the key directly? + maybeKey := InternalKey(name) + if s, ok := registry[maybeKey]; ok { + nameStatus := NameActive + if s.Name() != SettingName(maybeKey) { + // The user is invited to use the new name instead of the key. + nameStatus = NameRetired + } + return maybeKey, true, nameStatus + } + return "", false, NameActive } // 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 SettingName, forSystemTenant bool) (NonMaskedSetting, bool) { - // TODO(knz): handle names different from keys. - key := InternalKey(name) - return LookupForLocalAccessByKey(key, forSystemTenant) +func LookupForLocalAccess( + name SettingName, forSystemTenant bool, +) (NonMaskedSetting, bool, NameStatus) { + key, ok, nameStatus := NameToKey(name) + if !ok { + return nil, ok, nameStatus + } + s, ok := LookupForLocalAccessByKey(key, forSystemTenant) + return s, ok, nameStatus } // LookupForLocalAccessByKey returns a NonMaskedSetting by key. Used when a diff --git a/pkg/settings/settings_test.go b/pkg/settings/settings_test.go index 3c2e66361cad..d06a2375d476 100644 --- a/pkg/settings/settings_test.go +++ b/pkg/settings/settings_test.go @@ -346,7 +346,7 @@ 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.Name(), settings.ForSystemTenant) + result, ok, _ := settings.LookupForLocalAccess(s.Name(), settings.ForSystemTenant) if !ok { t.Fatalf("lookup(%s) failed", s.Name()) } @@ -357,7 +357,7 @@ 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.Name(), false /* forSystemTenant */) + result, ok, _ := settings.LookupForLocalAccess(s.Name(), false /* forSystemTenant */) if !ok { t.Fatalf("lookup(%s) failed", s.Name()) } @@ -368,7 +368,7 @@ 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.Name(), false /* forSystemTenant */) + _, ok, _ := settings.LookupForLocalAccess(s.Name(), false /* forSystemTenant */) if ok { t.Fatalf("lookup(%s) should have failed", s.Name()) } @@ -683,12 +683,12 @@ func TestCache(t *testing.T) { } func TestIsReportable(t *testing.T) { - if v, ok := settings.LookupForLocalAccess( + if v, ok, _ := settings.LookupForLocalAccess( "bool.t", settings.ForSystemTenant, ); !ok || !settings.TestingIsReportable(v) { t.Errorf("expected 'bool.t' to be marked as isReportable() = true") } - if v, ok := settings.LookupForLocalAccess( + if v, ok, _ := settings.LookupForLocalAccess( "sekretz", settings.ForSystemTenant, ); !ok || settings.TestingIsReportable(v) { t.Errorf("expected 'sekretz' to be marked as isReportable() = false") @@ -708,7 +708,7 @@ func TestOnChangeWithMaxSettings(t *testing.T) { sv := &settings.Values{} sv.Init(ctx, settings.TestOpaque) var changes int - s, ok := settings.LookupForLocalAccess(maxName, settings.ForSystemTenant) + s, ok, _ := settings.LookupForLocalAccess(maxName, settings.ForSystemTenant) if !ok { t.Fatalf("expected lookup of %s to succeed", maxName) } diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index 46c018afeab1..64f03a50fcb8 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -4793,7 +4793,7 @@ value if you rely on the HLC for accuracy.`, return nil, errors.AssertionFailedf("expected string value, got %T", args[0]) } name := settings.SettingName(strings.ToLower(string(s))) - setting, ok := settings.LookupForLocalAccess(name, evalCtx.Codec.ForSystemTenant()) + setting, ok, _ := settings.LookupForLocalAccess(name, evalCtx.Codec.ForSystemTenant()) if !ok { return nil, errors.Newf("unknown cluster setting '%s'", name) } @@ -4823,7 +4823,7 @@ value if you rely on the HLC for accuracy.`, return nil, errors.AssertionFailedf("expected string value, got %T", args[1]) } name := settings.SettingName(strings.ToLower(string(s))) - setting, ok := settings.LookupForLocalAccess(name, evalCtx.Codec.ForSystemTenant()) + setting, ok, _ := settings.LookupForLocalAccess(name, evalCtx.Codec.ForSystemTenant()) if !ok { return nil, errors.Newf("unknown cluster setting '%s'", name) } diff --git a/pkg/sql/set_cluster_setting.go b/pkg/sql/set_cluster_setting.go index d7c136cb26e0..ebc5aea39a50 100644 --- a/pkg/sql/set_cluster_setting.go +++ b/pkg/sql/set_cluster_setting.go @@ -156,10 +156,14 @@ func (p *planner) SetClusterSetting( ) (planNode, error) { name := settings.SettingName(strings.ToLower(n.Name)) st := p.EvalContext().Settings - setting, ok := settings.LookupForLocalAccess(name, p.ExecCfg().Codec.ForSystemTenant()) + setting, ok, nameStatus := settings.LookupForLocalAccess(name, p.ExecCfg().Codec.ForSystemTenant()) if !ok { return nil, errors.Errorf("unknown cluster setting '%s'", name) } + if nameStatus != settings.NameActive { + p.BufferClientNotice(ctx, settingNameDeprecationNotice(name, setting.Name())) + name = setting.Name() + } if err := checkPrivilegesForSetting(ctx, p, name, "set"); err != nil { return nil, err diff --git a/pkg/sql/set_var.go b/pkg/sql/set_var.go index 6b9b85723a89..f9e3a644a7dd 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(settings.SettingName(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 c2cb123f7e0f..102b5bfe7c62 100644 --- a/pkg/sql/show_cluster_setting.go +++ b/pkg/sql/show_cluster_setting.go @@ -152,14 +152,22 @@ func checkClusterSettingValuesAreEquivalent(localRawVal, kvRawVal []byte) error localVal, kvVal) } +func settingNameDeprecationNotice(oldName, newName settings.SettingName) pgnotice.Notice { + return pgnotice.Newf("the name %q is deprecated; use %q instead", oldName, newName) +} + func (p *planner) ShowClusterSetting( ctx context.Context, n *tree.ShowClusterSetting, ) (planNode, error) { name := settings.SettingName(strings.ToLower(n.Name)) - setting, ok := settings.LookupForLocalAccess(name, p.ExecCfg().Codec.ForSystemTenant()) + setting, ok, nameStatus := settings.LookupForLocalAccess(name, p.ExecCfg().Codec.ForSystemTenant()) if !ok { return nil, errors.Errorf("unknown setting: %q", name) } + if nameStatus != settings.NameActive { + p.BufferClientNotice(ctx, settingNameDeprecationNotice(name, setting.Name())) + name = setting.Name() + } if err := checkPrivilegesForSetting(ctx, p, name, "show"); err != nil { return nil, err diff --git a/pkg/sql/tenant_settings.go b/pkg/sql/tenant_settings.go index 0bf8db18a30c..ce33108c57b3 100644 --- a/pkg/sql/tenant_settings.go +++ b/pkg/sql/tenant_settings.go @@ -57,10 +57,14 @@ func (p *planner) AlterTenantSetClusterSetting( name := settings.SettingName(strings.ToLower(n.Name)) st := p.EvalContext().Settings - setting, ok := settings.LookupForLocalAccess(name, true /* forSystemTenant - checked above already */) + setting, ok, nameStatus := settings.LookupForLocalAccess(name, true /* forSystemTenant - checked above already */) if !ok { return nil, errors.Errorf("unknown cluster setting '%s'", name) } + if nameStatus != settings.NameActive { + p.BufferClientNotice(ctx, settingNameDeprecationNotice(name, setting.Name())) + name = setting.Name() + } // Error out if we're trying to set a system-only variable. if setting.Class() == settings.SystemOnly { return nil, pgerror.Newf(pgcode.InsufficientPrivilege, @@ -174,10 +178,14 @@ func (p *planner) ShowTenantClusterSetting( } name := settings.SettingName(strings.ToLower(n.Name)) - setting, ok := settings.LookupForLocalAccess(name, p.ExecCfg().Codec.ForSystemTenant()) + setting, ok, nameStatus := settings.LookupForLocalAccess(name, p.ExecCfg().Codec.ForSystemTenant()) if !ok { return nil, errors.Errorf("unknown setting: %q", name) } + if nameStatus != settings.NameActive { + p.BufferClientNotice(ctx, settingNameDeprecationNotice(name, setting.Name())) + name = setting.Name() + } // Error out if we're trying to call this from a non-system tenant or if // we're trying to set a system-only variable.