no major comments; all just nits or optional feedback

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @RaduBerinde)

-- commits, line 38 at r11:
does this need a release note?

pkg/settings/bool.go, line 49 at r10 (raw file):

// DecodeToString decodes and renders an encoded value.
func (b *BoolSetting) DecodeToString(encoded string) (string, error) {
	bv, err := b.DecodeValue(encoded)

i see it's not related to this PR, but what's the purpose of DecodeToString? it looks like it calls strconv.ParseBool and then calls strconv.FormatBool(b)

pkg/sql/show_cluster_setting.go, line 101 at r10 (raw file):

					}

					res = string(kvRawVal)

it seems fine, but i'm just wondering why this needed to change

pkg/sql/show_cluster_setting.go, line 129 at r10 (raw file):

	setting, ok := val.(settings.NonMaskedSetting)
	if !ok {
		return nil, errors.AssertionFailedf("setting is masked: %v", name)

is this supposed to be an assertion error, or would a user-friendly error be better?

pkg/sql/show_cluster_setting.go, line 143 at r10 (raw file):

}

func planShowClusterSetting(

would this more naturally be a method with a *planner receiver?

pkg/sql/show_cluster_setting.go, line 211 at r10 (raw file):

					}
					d = &tree.DInterval{Duration: duration.MakeDuration(v.Nanoseconds(), 0, 0)}
				case *settings.DurationSettingWithExplicitUnit:

could Duration and DurationWithInterval be combined into one case?

pkg/sql/tenant_settings.go, line 259 at r11 (raw file):

	}

	return &delayedNode{

why is this delayedNode needed here? could it instead use the node returned by planShowClusterSetting? and maybe change planShowClusterSetting so that it sets the name as we desire

this part could be done before the call to planShowClusterSetting

			// Evaluate the tenant ID expression.
			_, tenantID, err := resolveTenantID(p, typedTenantID)
			if err != nil {
				return nil, err
			}
			if err := assertTenantExists(ctx, p, tenantID); err != nil {
				return nil, err
			}

pkg/sql/logictest/testdata/logic_test/cluster_settings, line 227 at r11 (raw file):

onlyif config 3node-tenant
statement error SHOW CLUSTER SETTING FOR TENANT can only be called by system operators

would the term "system tenant" be more consistent with the error message below on line 236? (instead of "operator")

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde and @rafiss)

-- commits, line 38 at r11:

I'm not sure, since the tenant config override stuff is not accessible to end users? WDYT?

pkg/settings/bool.go, line 49 at r10 (raw file):

it's part of the interface. Other setting types do not use such a trivial function.

Also it checks for syntax errors in the encoded value, in this case e.g. it verifies that "wowow" causes an error and is not just rendered as-is.

pkg/sql/show_cluster_setting.go, line 101 at r10 (raw file):

Previously, the function was called showVersionSetting and it was responsible for returning the decoded value.

Now it's called getCurrentEncodedVersionSettingValue and is responsible for returning the encoded value.

Added a comment to clarify.

pkg/sql/show_cluster_setting.go, line 129 at r10 (raw file):

We'd be encountering a bug in Lookup. added a comment to explain.

pkg/sql/show_cluster_setting.go, line 143 at r10 (raw file):

I wanted to be more specific about the exact data that it needs, instead of leaking all the contents of planner into it. You'll notice that the body of the function doesn't need the planner, it's merely passed through from the constructor closure to the getEncodedValue function. I'd like this property (the fact the body doesn't need planner) to be maintained, which would be hard if the thing was passed as argument.

pkg/sql/show_cluster_setting.go, line 211 at r10 (raw file):

I tried but alas, DecodeValue is not part of the interface (it returns a different type for each instance) so when we combine the cases, the call to DecodeValue becomes invalid.

pkg/sql/tenant_settings.go, line 259 at r11 (raw file):

no, we can't call .Eval() (inside resolveTenantID) on the typed expr during planning, because it's the planning phase, not execution phase. The call to Eval must be delayed until execution (under startExec).

Alternatively, we could remove the call to resolveTenantID / ssertTenantExists entierly, and put the checks inside the SQL statement like what is done in delegate/show_all_cluster_settings.go. WDYT?

pkg/sql/logictest/testdata/logic_test/cluster_settings, line 227 at r11 (raw file):

Arguably it's the one saying "system tenant" that's inconsistent with the rest :)

lgtm!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @RaduBerinde, and @rafiss)

pkg/sql/tenant_settings.go, line 259 at r11 (raw file):

i see, thanks. yes it sounds good to keep the resolveTenantID in the exec phase like this

for consistency with show all i could see it being helpful to do the resolve/assert in SQL, but i don't feel strongly about it

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde and @rafiss)

pkg/sql/tenant_settings.go, line 259 at r11 (raw file):

ok, simplified.