Skip to content

Commit

Permalink
Merge #104449
Browse files Browse the repository at this point in the history
104449: sql: add default_value and origin to show cluster settings r=maryliag a=maryliag

Fixes https://cockroachlabs.atlassian.net/browse/CRDB-28519

**Commit 1**
This adds an API to the settings package to track the origin of the current value of each setting, such as whether it is set by the the default value, an explicit override or an external override.

**Commit 2**
Previously, there was no easy way to see default values for
cluster settings. This commit add the column for `default_value`
and `origin` to `crdb_internal.cluster_settings` and
the `show cluster settings` command.

Release note (sql change): Add columns `default_value` and
`origin` (default, override, external-override) to the
`show cluster settings` command.

Co-authored-by: David Taylor <[email protected]>
Co-authored-by: maryliag <[email protected]>
  • Loading branch information
3 people committed Jun 23, 2023
2 parents 1fd8581 + e64b6a4 commit a7c1494
Show file tree
Hide file tree
Showing 13 changed files with 140 additions and 24 deletions.
4 changes: 2 additions & 2 deletions pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant
Original file line number Diff line number Diff line change
Expand Up @@ -224,10 +224,10 @@ SELECT * FROM crdb_internal.session_trace WHERE span_idx < 0
----
span_idx message_idx timestamp duration operation loc tag message age

query TTTBT colnames
query TTTBTTT colnames
SELECT * FROM crdb_internal.cluster_settings WHERE variable = ''
----
variable value type public description
variable value type public description default_value origin

query TI colnames
SELECT * FROM crdb_internal.feature_usage WHERE feature_name = ''
Expand Down
2 changes: 1 addition & 1 deletion pkg/server/settings_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ func TestSettingsShowAll(t *testing.T) {
if len(rows) < 2 {
t.Fatalf("show all returned too few rows (%d)", len(rows))
}
const expColumns = 5
const expColumns = 7
if len(rows[0]) != expColumns {
t.Fatalf("show all must return %d columns, found %d", expColumns, len(rows[0]))
}
Expand Down
13 changes: 8 additions & 5 deletions pkg/server/settingswatcher/settings_watcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ func (s *SettingsWatcher) maybeSet(ctx context.Context, name string, sv settings
}
} else {
if !hasOverride {
s.setLocked(ctx, name, sv.val)
s.setLocked(ctx, name, sv.val, settings.OriginExplicitlySet)
}
}
}
Expand All @@ -309,7 +309,9 @@ type settingsValue struct {
const versionSettingKey = "version"

// set the current value of a setting.
func (s *SettingsWatcher) setLocked(ctx context.Context, key string, val settings.EncodedValue) {
func (s *SettingsWatcher) setLocked(
ctx context.Context, key string, 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
// BumpClusterVersion instead). The secondary tenants however, still rely
Expand All @@ -335,6 +337,7 @@ func (s *SettingsWatcher) setLocked(ctx context.Context, key string, val setting
if err := s.mu.updater.Set(ctx, key, val); err != nil {
log.Warningf(ctx, "failed to set setting %s to %s: %v", redact.Safe(key), val.Value, err)
}
s.mu.updater.SetValueOrigin(ctx, key, origin)
}

// setDefaultLocked sets a setting to its default value.
Expand All @@ -348,7 +351,7 @@ func (s *SettingsWatcher) setDefaultLocked(ctx context.Context, key string) {
Value: setting.EncodedDefault(),
Type: setting.Typ(),
}
s.setLocked(ctx, key, val)
s.setLocked(ctx, key, val, settings.OriginDefault)
}

// updateOverrides updates the overrides map and updates any settings
Expand Down Expand Up @@ -384,7 +387,7 @@ func (s *SettingsWatcher) updateOverrides(ctx context.Context) {
}
// A new override was added or an existing override has changed.
s.mu.overrides[key] = val
s.setLocked(ctx, key, val)
s.setLocked(ctx, key, val, settings.OriginExternallySet)
}

// Clean up any overrides that were removed.
Expand All @@ -395,7 +398,7 @@ func (s *SettingsWatcher) updateOverrides(ctx context.Context) {
// Reset the setting to the value in the settings table (or the default
// value).
if sv, ok := s.mu.values[key]; ok && !sv.tombstone {
s.setLocked(ctx, key, sv.val)
s.setLocked(ctx, key, sv.val, settings.OriginExplicitlySet)
} else {
s.setDefaultLocked(ctx, key)
}
Expand Down
11 changes: 11 additions & 0 deletions pkg/settings/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,15 @@ func (c *common) ErrorHint() (bool, string) {
return false, ""
}

func (c *common) getSlot() slotIdx {
return c.slot
}

// ValueOrigin returns the origin of the current value of the setting.
func (c *common) ValueOrigin(ctx context.Context, sv *Values) ValueOrigin {
return sv.getValueOrigin(ctx, c.slot)
}

// SetReportable indicates whether a setting's value can show up in SHOW ALL
// CLUSTER SETTINGS and telemetry reports.
//
Expand Down Expand Up @@ -114,6 +123,8 @@ type internalSetting interface {
isRetired() bool
setToDefault(ctx context.Context, sv *Values)

getSlot() slotIdx

// isReportable indicates whether the value of the setting can be
// included in user-facing reports such as that produced by SHOW ALL
// CLUSTER SETTINGS.
Expand Down
29 changes: 28 additions & 1 deletion pkg/settings/setting.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@

package settings

import "context"
import (
"context"
"fmt"
)

// Setting is the interface exposing the metadata for a cluster setting.
//
Expand Down Expand Up @@ -79,6 +82,9 @@ type NonMaskedSetting interface {
// ErrorHint returns a hint message to be displayed to the user when there's
// an error.
ErrorHint() (bool, string)

// ValueOrigin returns the origin of the current value.
ValueOrigin(ctx context.Context, sv *Values) ValueOrigin
}

// Class describes the scope of a setting in multi-tenant scenarios. While all
Expand Down Expand Up @@ -142,3 +148,24 @@ const (
// In short: "Go ahead but be careful."
Public
)

// ValueOrigin indicates the origin of the current value of a setting, e.g. if
// it is coming from the in-code default or an explicit override.
type ValueOrigin uint32

const (
// OriginDefault indicates the value in use is the default value.
OriginDefault ValueOrigin = iota
// OriginExplicitlySet indicates the value is has been set explicitly.
OriginExplicitlySet
// OriginExternallySet indicates the value has been set externally, such as
// via a host-cluster override for this or all tenant(s).
OriginExternallySet
)

func (v ValueOrigin) String() string {
if v > OriginExternallySet {
return fmt.Sprintf("invalid (%d)", v)
}
return [...]string{"default", "override", "external-override"}[v]
}
18 changes: 18 additions & 0 deletions pkg/settings/updater.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ type updater struct {
type Updater interface {
Set(ctx context.Context, key string, value EncodedValue) error
ResetRemaining(ctx context.Context)
SetValueOrigin(ctx context.Context, key string, origin ValueOrigin)
}

// A NoopUpdater ignores all updates.
Expand All @@ -83,6 +84,8 @@ func (u NoopUpdater) Set(ctx context.Context, key string, value EncodedValue) er
// 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) {}

// NewUpdater makes an Updater.
func NewUpdater(sv *Values) Updater {
if ignoreAllUpdates {
Expand Down Expand Up @@ -165,6 +168,13 @@ func (u updater) Set(ctx context.Context, key string, value EncodedValue) error
// ResetRemaining sets all settings not updated by the updater to their default values.
func (u updater) ResetRemaining(ctx context.Context) {
for k, v := range registry {

if _, hasOverride := u.m[k]; hasOverride {
u.sv.setValueOrigin(ctx, v.getSlot(), OriginExplicitlySet)
} else {
u.sv.setValueOrigin(ctx, v.getSlot(), OriginDefault)
}

if u.sv.NonSystemTenant() && v.Class() == SystemOnly {
// Don't try to reset system settings on a non-system tenant.
continue
Expand All @@ -174,3 +184,11 @@ 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) {
d, ok := registry[key]
if ok {
u.sv.setValueOrigin(ctx, d.getSlot(), origin)
}
}
10 changes: 10 additions & 0 deletions pkg/settings/values.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ type valuesContainer struct {
// current context (i.e. it is a SystemOnly setting and the container is for a
// tenant). Reading or writing such a setting causes panics in test builds.
forbidden [numSlots]bool

hasValue [numSlots]uint32
}

func (c *valuesContainer) setGenericVal(slot slotIdx, newVal interface{}) {
Expand Down Expand Up @@ -154,6 +156,14 @@ func (sv *Values) setInt64(ctx context.Context, slot slotIdx, newVal int64) {
}
}

func (sv *Values) setValueOrigin(ctx context.Context, slot slotIdx, origin ValueOrigin) {
atomic.StoreUint32(&sv.container.hasValue[slot], uint32(origin))
}

func (sv *Values) getValueOrigin(ctx context.Context, slot slotIdx) ValueOrigin {
return ValueOrigin(atomic.LoadUint32(&sv.container.hasValue[slot]))
}

// setDefaultOverride overrides the default value for the respective setting to
// newVal.
func (sv *Values) setDefaultOverride(slot slotIdx, newVal interface{}) {
Expand Down
11 changes: 10 additions & 1 deletion pkg/sql/crdb_internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -2054,7 +2054,9 @@ CREATE TABLE crdb_internal.cluster_settings (
value STRING NOT NULL,
type STRING NOT NULL,
public BOOL NOT NULL, -- whether the setting is documented, which implies the user can expect support.
description STRING NOT NULL
description STRING NOT NULL,
default_value STRING NOT NULL,
origin STRING NOT NULL -- the origin of the value: 'default' , 'override' or 'external-override'
)`,
populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error {
hasSqlModify, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.MODIFYSQLCLUSTERSETTING, p.User())
Expand Down Expand Up @@ -2093,12 +2095,19 @@ CREATE TABLE crdb_internal.cluster_settings (
strVal := setting.String(&p.ExecCfg().Settings.SV)
isPublic := setting.Visibility() == settings.Public
desc := setting.Description()
defaultVal, err := setting.DecodeToString(setting.EncodedDefault())
if err != nil {
return err
}
origin := setting.ValueOrigin(ctx, &p.ExecCfg().Settings.SV).String()
if err := addRow(
tree.NewDString(k),
tree.NewDString(strVal),
tree.NewDString(setting.Typ()),
tree.MakeDBool(tree.DBool(isPublic)),
tree.NewDString(desc),
tree.NewDString(defaultVal),
tree.NewDString(origin),
); err != nil {
return err
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/delegate/show_all_cluster_settings.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,12 @@ func (d *delegator) delegateShowClusterSettingList(

if stmt.All {
return d.parse(
`SELECT variable, value, type AS setting_type, public, description
`SELECT variable, value, type AS setting_type, public, description, default_value, origin
FROM crdb_internal.cluster_settings`,
)
}
return d.parse(
`SELECT variable, value, type AS setting_type, description
`SELECT variable, value, type AS setting_type, description, default_value, origin
FROM crdb_internal.cluster_settings
WHERE public IS TRUE`,
)
Expand Down
38 changes: 38 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/cluster_settings
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,44 @@ WHERE variable IN ('sql.defaults.default_int_size')
----
sql.defaults.default_int_size 4

query TTTT
SELECT variable, value, default_value, origin FROM [SHOW ALL CLUSTER SETTINGS]
WHERE variable IN ('sql.index_recommendation.drop_unused_duration')
----
sql.index_recommendation.drop_unused_duration 168h0m0s 168h0m0s default

statement ok
SET CLUSTER SETTING sql.index_recommendation.drop_unused_duration = '10s'

query TTTT
SELECT variable, value, default_value, origin FROM [SHOW ALL CLUSTER SETTINGS]
WHERE variable IN ('sql.index_recommendation.drop_unused_duration')
----
sql.index_recommendation.drop_unused_duration 10s 168h0m0s override

statement ok
RESET CLUSTER SETTING sql.index_recommendation.drop_unused_duration

query TTTT
SELECT variable, value, default_value, origin FROM [SHOW ALL CLUSTER SETTINGS]
WHERE variable IN ('sql.index_recommendation.drop_unused_duration')
----
sql.index_recommendation.drop_unused_duration 168h0m0s 168h0m0s default

user host-cluster-root

statement ok
ALTER TENANT ALL SET CLUSTER SETTING sql.index_recommendation.drop_unused_duration = '50s'

user root

onlyif config 3node-tenant-default-configs
query TTTT
SELECT variable, value, default_value, origin FROM [SHOW ALL CLUSTER SETTINGS]
WHERE variable IN ('sql.index_recommendation.drop_unused_duration')
----
sql.index_recommendation.drop_unused_duration 50s 168h0m0s external-override

user root

statement ok
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/logictest/testdata/logic_test/crdb_internal
Original file line number Diff line number Diff line change
Expand Up @@ -355,10 +355,10 @@ SELECT * FROM crdb_internal.session_trace WHERE span_idx < 0
----
span_idx message_idx timestamp duration operation loc tag message age

query TTTBT colnames
query TTTBTTT colnames
SELECT * FROM crdb_internal.cluster_settings WHERE variable = ''
----
variable value type public description
variable value type public description default_value origin

query TI colnames
SELECT * FROM crdb_internal.feature_usage WHERE feature_name = ''
Expand Down
Loading

0 comments on commit a7c1494

Please sign in to comment.