sql: remove references to closed multi-tenancy issues

[yuzefovich]

This commit updates a few places to remove now-closed multi-tenancy
issues. Two of those places are gated behind the system tenant, so the
issue doesn't matter, and they now use existing issue (even though it
doesn't really relate to the features in question). In one place we
choose to return "tenant cluster setting" not enabled error to make it
more clear (I was just bitten by this because we returned opaque
"unimplemented" error). In two other places in comments we update the
references to existing issues tracking the corresponding work.

Informs: #75569.
Epic: None

Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[yuzefovich]

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

---

pkg/sql/set_zone_config.go line 265 at r1 (raw file):

	if err := execCfg.RequireSystemTenantOrClusterSetting(SecondaryTenantZoneConfigsEnabled); err != nil {
		// Return an unimplemented error here instead of referencing the cluster

I think the change of the returned error here is the only contentious change in this PR. I was surprised by the error returned in shared-process MT mode, so at least in that mode I think we should return err. For separate-process MT it might make sense to return errorutil.*. Curious about your thoughts.

[knz]

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

---

pkg/sql/set_zone_config.go line 265 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I think the change of the returned error here is the only contentious change in this PR. I was surprised by the error returned in shared-process MT mode, so at least in that mode I think we should return err. For separate-process MT it might make sense to return errorutil.*. Curious about your thoughts.

I think we want to change the error returned by RequireXXX() to not use the word "tenant" and maybe move the name of the cluster setting to the DETAIL payload. That is, instead of

errors.Newf("tenant cluster setting %s disabled", setting.Name())

something like:

errors.WithDetailf(
  errors.WithHint(errors.New("operation is disabled within a virtual cluster",
  "Feature was disabled by the system operator."),
  "Feature flag: %s", setting.Name))

[yuzefovich]

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

---

pkg/sql/set_zone_config.go line 265 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

I think we want to change the error returned by RequireXXX() to not use the word "tenant" and maybe move the name of the cluster setting to the DETAIL payload. That is, instead of

errors.Newf("tenant cluster setting %s disabled", setting.Name())

something like:

errors.WithDetailf(
  errors.WithHint(errors.New("operation is disabled within a virtual cluster",
  "Feature was disabled by the system operator."),
  "Feature flag: %s", setting.Name))

Nice, I like it, done.

[knz]

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

[yuzefovich]

TFTR!

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)