sql: implement SHOW [ALL] CLUSTER SETTINGS FOR TENANT #77742

knz · 2022-03-13T22:23:36Z

All commits but the last 2 from #77740.
(Reviewers: only the last 2 commits belong to this PR.)

Informs #77471

Release justification: low risk, high benefit changes to existing functionality

cockroach-teamcity · 2022-03-13T22:23:56Z

This change is

knz · 2022-03-13T22:42:33Z

~~Note to self: need to introduce a decode function to decode the value column in the COALESCE clause.~~ (edit: done)

knz · 2022-03-16T13:12:41Z

Updated to reflect #77935.

knz · 2022-03-21T21:17:12Z

RFAL

rafiss

just had small comments

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

-- commits, line 22 at r15:
does this need a release note?

pkg/sql/delegate/show_all_cluster_settings.go, line 69 at r15 (raw file):

	if !d.evalCtx.Codec.ForSystemTenant() {
		return nil, pgerror.Newf(pgcode.InsufficientPrivilege,
			"SHOW CLUSTER SETTINGS FOR TENANT can only be called by system operators")

nit: is the term "on the system tenant" more familiar than "by system operatosr"

pkg/sql/delegate/show_all_cluster_settings.go, line 84 at r15 (raw file):

  isvalid AS (
    SELECT
      IF(tenant_id=0,

i think this would be more readable with:

CASE
  WHEN tenant_id=0 THEN ...
  WHEN tenant_id=1 THEN ...
  WHEN st.id IS NULL THEN ...
  ELSE 0
END

pkg/sql/delegate/show_all_cluster_settings.go, line 85 at r15 (raw file):

    SELECT
      IF(tenant_id=0,
         crdb_internal.force_error('22023', 'tenant ID must be non-zero'),

why do this validation / error creation in SQL rather than go code?

pkg/sql/delegate/show_all_cluster_settings.go, line 98 at r15 (raw file):

  crdb_internal.decode_cluster_setting(allsettings.variable,
     -- NB: careful not to coalesce with allsettings.value directly!
     -- This is the value fo the system tenant and is not relevant to other tenants.

nit: "fo"

pkg/sql/delegate/show_all_cluster_settings.go, line 107 at r15 (raw file):

  allsettings.type,
  ` + publicCol + `
  IF(tenantspecific.value IS NOT NULL, 'per-tenant-override',

i think CASE would be more readable here too

pkg/sql/delegate/show_all_cluster_settings.go, line 112 at r15 (raw file):

  allsettings.description
FROM
  (SELECT * FROM system.crdb_internal.cluster_settings ` + publicFilter + `) AS allsettings

since there already are CTEs in this query, i wonder if this would be more understandable if CTEs were used for allsettings and tenantspecific instead of subqueries?

pkg/sql/delegate/show_all_cluster_settings.go, line 113 at r15 (raw file):

FROM
  (SELECT * FROM system.crdb_internal.cluster_settings ` + publicFilter + `) AS allsettings
  LEFT JOIN (SELECT * FROM system.tenant_settings t, tenant_id WHERE t.tenant_id = tenant_id.tenant_id) AS tenantspecific ON

here and the above lines, would be a bit safer and easier to read this if the columns were named explicitly instead of using *

pkg/sql/sem/builtins/builtins.go, line 4632 at r14 (raw file):

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

should this be a user-friendly error?

pkg/sql/sem/builtins/builtins.go, line 4664 at r14 (raw file):

				)
				if !ok {
					return nil, errors.Newf("unknown cluster setting '%s'", name)

could this have a pgcode?

pkg/sql/sem/builtins/builtins.go, line 4668 at r14 (raw file):

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

should this be a user-friendly error?

pkg/sql/sem/builtins/builtins.go, line 4676 at r14 (raw file):

				return tree.NewDString(repr), nil
			},
			Info:       "Returns the encoded default value of the given cluster setting.",

remove "default" and change "encoded" to "decoded" (or reword as you like)

knz

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

-- commits, line 22 at r15: