settings,*: update the settings.Setting API

[knz]

Fixes #108903.
Informs: #108508
Informs: #109046
Epic: CRDB-27642

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:

type Setting interface {
  // Key returns the name of the specific cluster setting.
  Key() string
}

After this change:

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

[cockroach-teamcity]

This change is 

[yuzefovich]

Nice! I have some nits that are related to the following questions:

• do we want to apply sql.defaults restriction to only keys, only names, or both?
• it seems like in some unit tests we could be using Name in more places. Is there a reason why not? It seems like it'd be nice to use Name unless it doesn't work because we need InternalKey type.

Reviewed 70 of 70 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @stevendanna)

---

pkg/cli/gen.go line 265 at r1 (raw file):

				alterRoleLink = `<a href="alter-role.html"><code>ALTER ROLE... SET</code></a>`
			}
			if strings.Contains(string(name), "sql.defaults") {

Shouldn't this be key? Or perhaps both?

---

pkg/server/admin.go line 2041 at r1 (raw file):

			for _, k := range settingsKeys {
				if slices.Contains(settings.ConsoleKeys(), k) {
					newSettingsKeys = append(settingsKeys, k)

I think this should be newSettingKeys = append(newSettingKeys, k).

---

pkg/server/settingswatcher/settings_watcher.go line 269 at r1 (raw file):

		}
		if setting.Class() != settings.TenantWritable {
			log.Warningf(ctx, "ignoring read-only setting %s", settingKey)

nit: should we use Name here?

---

pkg/server/tenantsettingswatcher/overrides_store.go line 149 at r1 (raw file):

		after = append(after, setting)
	}
	// Skip any existing setting for this name.

nit: perhaps replace "name" with "setting key" or something.

---

pkg/settings/registry.go line 320 at r1 (raw file):

// NameToKey returns the key associated with a setting name.
func NameToKey(name SettingName) (InternalKey, bool) {
	// TODO(...): improve this.

nit: I think our style guide says always include the github username in TODOs even if you're personally not planning to address it.

---

pkg/settings/registry.go line 349 at r1 (raw file):

// LookupForReporting returns a Setting by name. Used when a setting is being
// retrieved for reporting.

nit: perhaps insert an empty line (for symmetry with the comment below).

---

pkg/sql/show_cluster_setting.go line 168 at r1 (raw file):

	}

	if strings.HasPrefix(n.Name, "sql.defaults") {

nit: perhaps use name here too.

---

pkg/ccl/multitenantccl/tenantcostclient/tenant_side_test.go line 1462 at r1 (raw file):

	}
	for _, setting := range settings {
		sysDB.Exec(t, fmt.Sprintf("ALTER TENANT ALL SET CLUSTER SETTING %s = $1", setting.InternalKey()), setting.Default()*100)

nit: why not use Name here?

---

pkg/sql/show_test.go line 1217 at r1 (raw file):

	for rows.Next() {
		var settingKey, sType, desc string

nit: technically speaking, shouldn't it be named settingName? IIUC SHOW ALL CLUSTER SETTINGS uses crdb_internal.cluster_settings virtual table which is now updated to return the setting name.

Relatedly, perhaps we want to run the linter on both the key and the name?

---

pkg/sql/catalog/schematelemetry/schema_telemetry_test.go line 67 at r1 (raw file):

	qSet = fmt.Sprintf(`SET CLUSTER SETTING %s = '* * * * *'`,
		schematelemetrycontroller.SchemaTelemetryRecurrence.InternalKey())

nit: this also seems like it could be Name.

---

pkg/upgrade/upgrades/ensure_sql_schema_telemetry_schedule_test.go line 99 at r1 (raw file):

		// Check that the schedule can have its recurrence altered.
		tdb.Exec(t, fmt.Sprintf(`SET CLUSTER SETTING %s = '* * * * *'`,
			schematelemetrycontroller.SchemaTelemetryRecurrence.InternalKey()))

nit: ditto here and below.

[knz]

do we want to apply sql.defaults restriction to only keys, only names, or both?

Let's do both.

it seems like in some unit tests we could be using Name in more places. Is there a reason why not?

There's no good reason. Let's do it.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @stevendanna and @yuzefovich)

---

pkg/cli/gen.go line 265 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Shouldn't this be key? Or perhaps both?

Done.

---

pkg/server/admin.go line 2041 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I think this should be newSettingKeys = append(newSettingKeys, k).

Good catch. Fixed.

---

pkg/server/settingswatcher/settings_watcher.go line 269 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: should we use Name here?

We could, but it's not in scope. I don't feel it's worth performing a lookup.

---

pkg/server/tenantsettingswatcher/overrides_store.go line 149 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: perhaps replace "name" with "setting key" or something.

Done.

---

pkg/settings/registry.go line 320 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: I think our style guide says always include the github username in TODOs even if you're personally not planning to address it.

Done.

---

pkg/settings/registry.go line 349 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: perhaps insert an empty line (for symmetry with the comment below).

Done.

---

pkg/sql/show_cluster_setting.go line 168 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: perhaps use name here too.

Done.

---

pkg/ccl/multitenantccl/tenantcostclient/tenant_side_test.go line 1462 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: why not use Name here?

IMHO in tests it doesn't matter as much. But ok - fixed.

---

pkg/sql/show_test.go line 1217 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: technically speaking, shouldn't it be named settingName? IIUC SHOW ALL CLUSTER SETTINGS uses crdb_internal.cluster_settings virtual table which is now updated to return the setting name.

Relatedly, perhaps we want to run the linter on both the key and the name?

Actually the linter is right here -- it only needs to operate on names, which is the only thing that's user-facing. So the logic was right from the start.
Just needed to rename settingKey -> settingName here. Done.

---

pkg/sql/catalog/schematelemetry/schema_telemetry_test.go line 67 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: this also seems like it could be Name.

Indeed. Fixed.

---

pkg/upgrade/upgrades/ensure_sql_schema_telemetry_schedule_test.go line 99 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: ditto here and below.

Done.

[yuzefovich]

Reviewed 15 of 15 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz and @stevendanna)

---

pkg/kv/kvpb/api.proto line 3259 at r2 (raw file):

}

// TenantSetting contains the name and value of a tenant setting.

nit: perhaps s/name/internal key/.

---

pkg/server/serverpb/admin.proto line 620 at r2 (raw file):

      bool public = 4;
      google.protobuf.Timestamp last_updated = 5 [(gogoproto.nullable) = true, (gogoproto.stdtime) = true];
      string name = 6; // for display purposes.

nit: this comment is included in generated docs, should we make it a full sentence that starts with the capital?

[knz]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @stevendanna)

---

pkg/kv/kvpb/api.proto line 3259 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: perhaps s/name/internal key/.

Done.

---

pkg/server/serverpb/admin.proto line 620 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: this comment is included in generated docs, should we make it a full sentence that starts with the capital?

Done.

[knz]

TFYR!

bors r=yuzefovich

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)