sql: implement the logic for ALTER TENANT SET CLUSTER SETTING

[knz]

Informs #77471

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

[cockroach-teamcity]

This change is 

[RaduBerinde]

Thanks for working on this!

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

---

pkg/sql/set_cluster_setting.go, line 379 at r6 (raw file):

	}

	if knobs := execCfg.TenantTestingKnobs; knobs != nil && knobs.ClusterSettingsUpdater != nil {

I don't think this block makes sense to run when we are setting a tenant override.

---

pkg/sql/set_cluster_setting.go, line 660 at r6 (raw file):

// settingWriter is an interface that abstracts the setting change logic
// over local and remote changes.
type settingWriter interface {

I am skeptical about the value of having this interface. The two code paths are very different and pretty well separated in the code; we're only saving the duplication of a few lines as far as I can tell, while making following the overall logic much harder.

At the very least, versionUpgradeHook should not be part of this interface (the caller, runMigrationsAndUpgradeVersion, only runs for the existing case).

---

pkg/sql/tenant_settings.go, line 49 at r6 (raw file):

	ctx context.Context, n *tree.AlterTenantSetClusterSetting,
) (planNode, error) {
	// Changing cluster settings for other tenants is a more

This statement should only be allowed if we are the system tenant.

---

pkg/sql/tenant_settings.go, line 74 at r6 (raw file):

	st := p.EvalContext().Settings
	v, ok := settings.Lookup(name, settings.LookupForLocalAccess, p.ExecCfg().Codec.ForSystemTenant())

Here we only want to allow tenant-visible settings, so we should always pass false for the last argument, or check that it is not a system setting if we want a better error message (and please add a test for this case).

---

pkg/util/log/eventpb/misc_sql_events.proto, line 44 at r6 (raw file):

// SetTenantClusterSetting is recorded when a cluster setting override
// is changed for another tenant.

[nit] is changed (either for a particular tenant, or for all tenants).

[knz]

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

---

pkg/sql/set_cluster_setting.go, line 379 at r6 (raw file):

Previously, RaduBerinde wrote…

I don't think this block makes sense to run when we are setting a tenant override.

Good point. Updated.

---

pkg/sql/set_cluster_setting.go, line 660 at r6 (raw file):

The two code paths are very different

In which way are they different?

and pretty well separated in the code

Where's the separation?

we're only saving the duplication of a few lines as far as I can tell

I tried otherwise and the change resulted in a large file with more than a hundred lines. I think the risk to get divergence in the common path if we duplicate the logic is high.

while making following the overall logic much harder.

Maybe we can help that with documentation.

At the very least, versionUpgradeHook should not be part of this interface (the caller, runMigrationsAndUpgradeVersion, only runs for the existing case).

What do you mean? versionUpgradeHook is called every time the version setting is updated. Secondary tenant have a version setting too!

Even though, as explained in #77733, we're not implementing version setting changes in the current PR, the purpose of the interface I'm introducing here is to make that feature possible later, so that the separate implementation of the interface can provide the new way to run the SQL migrations when changing the version settings for another tenant.

In the next commit, this method interface is

---

pkg/sql/tenant_settings.go, line 49 at r6 (raw file):

Previously, RaduBerinde wrote…

This statement should only be allowed if we are the system tenant.

This is already checked below (and in unit tests).

  if ¬p.execCfg.Codec.ForSystemTenant() {
    return ∅, pgerror.Newf(pgcode.InsufficientPrivilege,
      "ALTER TENANT can only be called by system operators")
  }

do you mean I should move this check up in the function?

---

pkg/sql/tenant_settings.go, line 74 at r6 (raw file):

Previously, RaduBerinde wrote…

Here we only want to allow tenant-visible settings, so we should always pass false for the last argument, or check that it is not a system setting if we want a better error message (and please add a test for this case).

There's a test just below:

  if setting.Class() ≡ settings.SystemOnly {
    return ∅, pgerror.Newf(pgcode.InsufficientPrivilege,
      "%s is a system-only setting and must be set in the admin tenant using SET CLUSTER SETTING", name)
  }

and it's also already exercised in tests.

[RaduBerinde]

---

pkg/sql/set_cluster_setting.go, line 660 at r6 (raw file):

Previously, knz (kena) wrote…

The two code paths are very different

In which way are they different?

and pretty well separated in the code

Where's the separation?

we're only saving the duplication of a few lines as far as I can tell

I tried otherwise and the change resulted in a large file with more than a hundred lines. I think the risk to get divergence in the common path if we duplicate the logic is high.

while making following the overall logic much harder.

Maybe we can help that with documentation.

At the very least, versionUpgradeHook should not be part of this interface (the caller, runMigrationsAndUpgradeVersion, only runs for the existing case).

What do you mean? versionUpgradeHook is called every time the version setting is updated. Secondary tenant have a version setting too!

Even though, as explained in #77733, we're not implementing version setting changes in the current PR, the purpose of the interface I'm introducing here is to make that feature possible later, so that the separate implementation of the interface can provide the new way to run the SQL migrations when changing the version settings for another tenant.

In the next commit, this method interface is

We are not implementing tenant settings here (they are already implemented). We are implementing tenant setting overrides. Version is not something that makes sense to override.

Maybe @ajwerner can chime in here too.

[knz]

We are not implementing tenant settings here

Hm that's right.

What's the logic envisioned to change a tenant's own local customization "remotely"?

[RaduBerinde]

What's the logic envisioned to change a tenant's own local customization "remotely"?

I might be misunderstanding your question. Are you asking how the overrides work on the tenant side? It's implemented as described in the RFC (#73349): the tenant KV connector monitors the overrides (via an InternalServer streaming API) and the settings watcher on the tenant side integrates with the tenant connector (via the setingswatcher.OverridesMonitor interface).

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

---

pkg/sql/set_cluster_setting.go, line 660 at r6 (raw file):

In which way are they different?

ALTER TENANT just has to write to the tenant_settings table. Not much else from the SET CLUSTER SETTINGS code applies: no special version handling, no waiting until the setting becomes "in effect". The only common thing is the encoding of the setting value into something that can be written to the table (which can be easily separated into a common routine).

Where's the separation?

I just mean that the AlterTenant node is separate, it's not like there's some common entry point which has to at some point "branch out" into the two possibilities.

---

pkg/sql/tenant_settings.go, line 49 at r6 (raw file):

Previously, knz (kena) wrote…

This is already checked below (and in unit tests).

  if ¬p.execCfg.Codec.ForSystemTenant() {
    return ∅, pgerror.Newf(pgcode.InsufficientPrivilege,
      "ALTER TENANT can only be called by system operators")
  }

do you mean I should move this check up in the function?

Ah, right, let's just move it up so it's next to this check, and it's as early as possible. And the rest of the code should not look at ForSystemTenant() again, as it can only be true.

---

pkg/sql/tenant_settings.go, line 74 at r6 (raw file):

Previously, knz (kena) wrote…

There's a test just below:

  if setting.Class() ≡ settings.SystemOnly {
    return ∅, pgerror.Newf(pgcode.InsufficientPrivilege,
      "%s is a system-only setting and must be set in the admin tenant using SET CLUSTER SETTING", name)
  }

and it's also already exercised in tests.

Ah, great, sorry for missing it.

[knz]

Ok I am just discovering that we're missing out on an important use case, let's resume this conversation in #77935.

I will simplify the current PR accordingly and we can do a separate PR when we tackle the new issue.

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

---

pkg/sql/set_cluster_setting.go, line 660 at r6 (raw file):

The only common thing is the encoding of the setting value into something that can be written to the table (which can be easily separated into a common routine).

But that's the change I have implemented here. The common code is now reusable in both places. What is missing?

---

pkg/sql/tenant_settings.go, line 49 at r6 (raw file):

Previously, RaduBerinde wrote…

Ah, right, let's just move it up so it's next to this check, and it's as early as possible. And the rest of the code should not look at ForSystemTenant() again, as it can only be true.

Done.

---

pkg/sql/tenant_settings.go, line 74 at r6 (raw file):

Previously, RaduBerinde wrote…

Ah, great, sorry for missing it.

Moved up higher.

[RaduBerinde]

---

pkg/sql/set_cluster_setting.go, line 660 at r6 (raw file):

Previously, knz (kena) wrote…

The only common thing is the encoding of the setting value into something that can be written to the table (which can be easily separated into a common routine).

But that's the change I have implemented here. The common code is now reusable in both places. What is missing?

Right, but the way you did it was to extract the non-common code behind an interface. The alternative I'm proposing is that the common code would be extracted into a few helper routines and the two codepaths would be otherwise unrelated. It's possible I'm missing some details that would make this harder than it seems, so it's up to you.

[RaduBerinde]

---

pkg/sql/set_cluster_setting.go, line 660 at r6 (raw file):

Previously, RaduBerinde wrote…

Right, but the way you did it was to extract the non-common code behind an interface. The alternative I'm proposing is that the common code would be extracted into a few helper routines and the two codepaths would be otherwise unrelated. It's possible I'm missing some details that would make this harder than it seems, so it's up to you.

Looking at it another way - we're implementing a statement which upserts or deletes a row in a table, and that's all it does. Does that justify shuffling around (and complicating) hundreds of lines of existing code?

[knz]

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

---

pkg/sql/set_cluster_setting.go, line 660 at r6 (raw file):

Previously, RaduBerinde wrote…

Looking at it another way - we're implementing a statement which upserts or deletes a row in a table, and that's all it does. Does that justify shuffling around (and complicating) hundreds of lines of existing code?

So to start with, the first commit in this PR is generally beneficial because it transforms nearly a thousand lines of code into many smaller functions, each with a clearer purpose.

With that infrastructure in place, the common path between SET CLUSTER SETTING and its ALTER TENANT variant is this:

• writeSettingInternal()
• writeDefaultSettingValue()
• half of writeNonDefaultSettingValue()
• toSettingString()

1. This is not a full hundred lines, but it's close to it. I do not feel comfortable copy-pasting that near hundred lines for the benefit of ALTER TENANT SET CLUSTER SETTING, because some of that logic could then diverge.

2. I stand by my point that we will likely want to solve sql: SHOW SETTING FOR TENANT(S) is not completely designed (and perhaps ALTER TENANT SET CLUSTER SETTING too) #77935 / sql: enable changing the version setting of a tenant from another tenant #77733 at some point. If/when that happens, then 100% of writeNonDefaultSettingValue() can be reused on both paths.

[RaduBerinde]

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

---

pkg/sql/set_cluster_setting.go, line 660 at r6 (raw file):

Previously, knz (kena) wrote…

So to start with, the first commit in this PR is generally beneficial because it transforms nearly a thousand lines of code into many smaller functions, each with a clearer purpose.

With that infrastructure in place, the common path between SET CLUSTER SETTING and its ALTER TENANT variant is this:

• writeSettingInternal()
• writeDefaultSettingValue()
• half of writeNonDefaultSettingValue()
• toSettingString()

1. This is not a full hundred lines, but it's close to it. I do not feel comfortable copy-pasting that near hundred lines for the benefit of ALTER TENANT SET CLUSTER SETTING, because some of that logic could then diverge.

2. I stand by my point that we will likely want to solve sql: SHOW SETTING FOR TENANT(S) is not completely designed (and perhaps ALTER TENANT SET CLUSTER SETTING too) #77935 / sql: enable changing the version setting of a tenant from another tenant #77733 at some point. If/when that happens, then 100% of writeNonDefaultSettingValue() can be reused on both paths.

Separating into functions is good, but moving some functions behind an interface is a non-trivial complication (I don't mean a technical complication, it's pretty easy to achieve in the code; I mean a conceptual complication - it leads to pieces of the code that are "generic" w.r.t the interface, so to understand them you need to have a concept of what the interface is and does in the back of your head).

What your analysis above is missing is that in the ALTER TENANT case, most of those common routines reduce to little else on top of calling the interface. Here, I gave what I propose a shot: RaduBerinde@aa79281

This is how little code we actually need, and it's mostly new code. The only thing I copy-pasted is the block of code below, which could be easily separated into a common routine:

		reportedValue = tree.AsStringWithFlags(n.value, tree.FmtBareStrings)
		value, err := n.value.Eval(params.p.EvalContext())
		if err != nil {
			return err
		}
		encoded, err := toSettingString(params.ctx, n.st, n.name, n.setting, value)

[knz]

RFAL

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

---

pkg/sql/set_cluster_setting.go, line 660 at r6 (raw file):

Previously, RaduBerinde wrote…

Separating into functions is good, but moving some functions behind an interface is a non-trivial complication (I don't mean a technical complication, it's pretty easy to achieve in the code; I mean a conceptual complication - it leads to pieces of the code that are "generic" w.r.t the interface, so to understand them you need to have a concept of what the interface is and does in the back of your head).

What your analysis above is missing is that in the ALTER TENANT case, most of those common routines reduce to little else on top of calling the interface. Here, I gave what I propose a shot: RaduBerinde@aa79281

This is how little code we actually need, and it's mostly new code. The only thing I copy-pasted is the block of code below, which could be easily separated into a common routine:

		reportedValue = tree.AsStringWithFlags(n.value, tree.FmtBareStrings)
		value, err := n.value.Eval(params.p.EvalContext())
		if err != nil {
			return err
		}
		encoded, err := toSettingString(params.ctx, n.st, n.name, n.setting, value)

ok, that works.

[RaduBerinde]

Thanks!

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

[knz]

bors r=RaduBerinde

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[arulajmani]

Hey @knz are we planning on backporting this PR to release-22.1? I was trying to backport a PR which makes use of this -- #79160.

[arulajmani]

blathers backport release-22.1