This change is 

Looking good. Some nits below and confusion about the <major>.<minor>.<unstable> syntax. Perhaps we should use <major>.<minor>-unstable<unstable>.

Review status: 0 of 32 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

pkg/base/cluster_version.go, line 43 at r1 (raw file):

	// VersionBase is the empty version and corresponds to any binary
	// older than 1.0.1, though these binaries won't know anything

This is mildly confusing. 1.0.1 doesn't correspond to the actual 1.0.1 release, right?

pkg/base/cluster_version.proto, line 32 at r1 (raw file):

  // Support for that functionality is guaranteed by the ratchet
  // of minimum_version.
  roachpb.Version use_version = 2 [(gogoproto.nullable) = false];

Bikeshed: should this be named inuse_version? The comment even uses the term in use.

pkg/migration/version.go, line 31 at r1 (raw file):

)

func parseAndValidateVersion(s string) (roachpb.Version, error) {

Should this method be roachpb.ParseVersion or something like that? Seems strange to have Version.String() in one package and the parsing in another.

pkg/migration/version.go, line 52 at r1 (raw file):

	c.Major = int32(ints[0])
	c.Minor = int32(ints[1])
	c.Unstable = int32(ints[2])

The Version proto has 4 fields, yet we only parse 3. I feel that this is part of my confusion mentioned in another comment.

pkg/settings/statemachine.go, line 62 at r1 (raw file):

// but not semantically. Users must call Validate with the authoritative
// previous state, the suggested state transition, and then overwrite the state
// machine with the result.

I haven't fully grok'ed the usage here, so apologies if this is misguided: did you consider adding a ValidationFn to the existing *Settings? I would think most of the above could be had by such a validation function hook. For the cluster version, I imagine a validation function on a StringSetting would be sufficient. It is very possible that I'm missing a constraint that is dictating this approach.

pkg/sql/show.go, line 96 at r1 (raw file):

					"SELECT value FROM system.settings WHERE name = $1",
					name,
				)

I thought SHOW CLUSTER SETTING was already a consistent read. @dt?

Comments from Reviewable

I like <maj>.<min>-unstable<x>. Will wait for some additional reviews before I get back to work on this.

Review status: 0 of 32 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

pkg/base/cluster_version.go, line 43 at r1 (raw file):

I think most of your confusion comes from the fact that version=major.minor.patch.unstable, but we always have patch==0 (RFC wants this) but don't want to print the patch version. I would be happy to turn the patch proto field into a reserved one and rip it out of the logic for now.

pkg/settings/statemachine.go, line 62 at r1 (raw file):

a validation doesn't cut it because the new value is the result of the validation. You also need the input to always be consistent or your output won't be (potentially breaking monotonicity of min_version). Plus you have a diverging internal (what's in the table) and external representation (what's showed to the client) which needs to be translated both ways. I initially tried exactly what you're describing but, yeah, it's more complicated to the point where it's easier to just make a new setting.

pkg/sql/show.go, line 96 at r1 (raw file):

You can see from the adjacent code that it isn't. It just reads the setting variable, but that isn't consistent. I think I would want to make show cluster setting X always read from the table, but wanted to keep the initial changes focused in case that's not desired.

I think though that if you show all cluster settings, you get a consistent read, maybe?

return p.delegateQuery(ctx, "SHOW CLUSTER SETTINGS",
			"TABLE crdb_internal.cluster_settings", nil, nil)

I also feel that I shouldn't be hooking into system.settings here but I think we're already below the level of abstraction at which crdb_internal is available.

Comments from Reviewable

Review status: 0 of 32 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

pkg/settings/statemachine.go, line 62 at r1 (raw file):

Ack. I wonder if there is a better term for this than "state machine". Yeah, it is a state machine, but so are so many other things. I guess the term feels to general purpose.

pkg/sql/show.go, line 96 at r1 (raw file):

Yeah, I'd want show cluster setting X to always read from the table. @dt Any reason it isn't already doing that?

Comments from Reviewable

Review status: 0 of 32 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

pkg/settings/statemachine.go, line 62 at r1 (raw file):

I find it pretty spot on and I don't like bikeshedding enough to participate, but if you come up with a stellar suggestion I'll be happy to rename.

pkg/sql/show.go, line 96 at r1 (raw file):

There's a straightforward reason, which is that nothing is written to the table unless the default is changed. This is actually a problem I need to address, as every cluster should always have the version setting as a running node can't really guess a default. It could be made workable because all nodes that know about versioning could persist their running versions and use that as the default, but this only seems to invite further complication.

This means that cluster init has to populate at least that setting, at which point it might as well populate all of them.

Comments from Reviewable

LGTM

Reviewed 35 of 35 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

pkg/settings/statemachine.go, line 62 at r1 (raw file):

I also don't like StatemachineSetting, but my best alternative, TransitionSetting, isn't very good either. I would, however, spell StatemachineSetting with a capital M.

Comments from Reviewable

(only looked at the settings part)

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

pkg/settings/statemachine.go, line 31 at r1 (raw file):

// For (a nonsensical) example, a StatemachineSetting can be used to maintain an
// encoded protobuf containing an integer that the user can only increment by 3
// if the int is odd and by 2 if it is equal. More generally, the setting starts

nit: s/equal/even/ right?

pkg/settings/updater.go, line 58 at r1 (raw file):

// Set attempts to parse and update a setting and notes that it was updated.
func (u Updater) Set(key string, rawValue string, vt string) error {

huh, is this a style rule I missed?

Comments from Reviewable

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

pkg/sql/show.go, line 96 at r1 (raw file):

yeah, the settings design in the initial rfc basically sees the table as only for overrides of defaults:
The decision to only persist explicit overrides was to make it easier to tune defaults -- otherwise we'd need migrations to rewrite the settings table if we discover a better value for some default, making tuning more expensive (and we'd also need to, of course, track which settings had explicit vs default values to do that migration correctly).

select * from system.settings gets you a consistent read of the current non-default settings, but show cluster setting shows you whatever a node is currently using -- which is probably whatever its hardcoded default is, but might be its current, cached, override.

Comments from Reviewable

TFTRs! Merging on green.

Review status: 13 of 33 files reviewed at latest revision, 9 unresolved discussions, some commit checks pending.

pkg/base/cluster_version.proto, line 32 at r1 (raw file):

I'll leave as is for now, keeping the wording congruent with the RFC.

pkg/migration/version.go, line 31 at r1 (raw file):

Good point, done.

pkg/migration/version.go, line 52 at r1 (raw file):

I changed the version to <major>.<minor>-<unstable> as you suggested. I think that works a lot better.

pkg/settings/statemachine.go, line 31 at r1 (raw file):

Yep, thanks. Fixed.

pkg/settings/statemachine.go, line 62 at r1 (raw file):

StateMachineSetting it is (until anyone feels strongly enough and swoops in for a rename, anyway).

pkg/settings/updater.go, line 58 at r1 (raw file):

Nope, just a spurious diff. Reverted.

pkg/sql/show.go, line 96 at r1 (raw file):

That distinction between system.settings and SHOW CLUSTER SETTINGS makes some sense, thanks @dt!
I'll punt on addressing this fully in this PR. @m-schneider and I will look at how to go about the default setting separately. Seems relatively clear to me that we'll just want to make sure there's always a persisted cluster version. What we'll have to figure out is if we're going to change anything beyond that.

Comments from Reviewable

Review status: 13 of 33 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

pkg/sql/show.go, line 96 at r1 (raw file):

For the diagnostic reporting setting, semantically we wanted a default of true, but since settings need to be allowed to return defaults until they are read and didn't want to risk ignoring an explicit false and betraying user trust, we went with a default of false with a startup migration writing an explicit true (optInToDiagnosticsStatReporting in migrations.go). This feels pretty similar.

Comments from Reviewable

Does ExposedClusterVersion have to be part of a StoreConfig rather than a package-scoped variable? It's going to be fairly awkward to plumb into the allocator scorer methods.

I responded to this offline, but for posterity: I feel fairly strongly that the less singletons, the better. The cluster version setting var is a particularly annoying, because due to it it's impossible to run mixed version cluster tests unless we're going straight for acceptance tests (in which case we have to juggle binary versions, etc, and Docker pain). Plumbing can certainly be annoying, but I think it's the lesser of two evils almost always, and there are some ways to make it more pleasant which I'm happy to contribute to moving forward.