Merge #57118 #57155 #57156

57118: migration,server: plumb in initial version to the migration manager r=irfansharif a=irfansharif

It'll let us return early, and makes the manager "more functional" in its
behavior. 

We also promote the clusterversion type to a first-class citizen in the
external facing API for pkg/migration. This package concerns itself with
migrations between cluster versions and we should have our API reflect as much.

The proto changes are safe, we haven't had a major release with the previous
proto definitions.

Release note: None

57155: clusterversion,*: remove VersionContainsEstimatesCounter r=irfansharif a=irfansharif

This, and all surrounding migration code and tests, are now safe to
remove. It mostly served as documentation, which we've moved to the
field itself. Part of #47447. Fixes #56401.

(While here, Let's also tell git that `versionkey_string.go` is a
generated file.)

Release note: None

---

First commit is from #57154.

57156: sql,clusterversion: remove VersionAuthLocalAndTrustRejectMethods r=irfansharif a=irfansharif

It's an old cluster version, introduced in the 19.2 release cycle. It's
now safe to remove. Part of #47447. Fixes #56398.


Release note: None

---

See only last commit. First two are from #57154 and #57155 respectively.

Co-authored-by: irfan sharif <[email protected]>

pkg/ccl/gssapiccl/gssapi.go

@@ -21,7 +21,6 @@ import (
 	"unsafe"
 
 	"github.com/cockroachdb/cockroach/pkg/ccl/utilccl"
-	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire"
@@ -207,5 +206,5 @@ func checkEntry(entry hba.Entry) error {
 }
 
 func init() {
-	pgwire.RegisterAuthMethod("gss", authGSS, clusterversion.Version19_1, hba.ConnHostSSL, checkEntry)
+	pgwire.RegisterAuthMethod("gss", authGSS, hba.ConnHostSSL, checkEntry)
 }

pkg/clusterversion/.gitattributes

@@ -0,0 +1 @@
+versionkey_string.go binary

pkg/clusterversion/cluster_version.proto

@@ -9,7 +9,7 @@
 // licenses/APL.txt.
 
 syntax = "proto3";
-package cockroach.base;
+package cockroach.clusterversion;
 option go_package = "clusterversion";
 
 import "roachpb/metadata.proto";

pkg/clusterversion/cockroach_versions.go

@@ -115,44 +115,15 @@ type VersionKey int
 const (
 	_ VersionKey = iota - 1 // want first named one to start at zero
 
-	// Version19_1 is CockroachDB v19.1. It's used for all v19.1.x patch
-	// releases.
-	Version19_1
-
 	// v20.1 versions.
 	//
-	// VersionContainsEstimatesCounter is
-	// https://github.com/cockroachdb/cockroach/pull/37583.
-	//
-	// MVCCStats.ContainsEstimates has been migrated from boolean to a
-	// counter so that the consistency checker and splits can reset it by
-	// returning -ContainsEstimates, avoiding racing with other operations
-	// that want to also change it.
-	//
-	// The migration maintains the invariant that raft commands with
-	// ContainsEstimates zero or one want the bool behavior (i.e. 1+1=1).
-	// Before the cluster version is active, at proposal time we'll refuse
-	// any negative ContainsEstimates plus we clamp all others to {0,1}.
-	// When the version is active, and ContainsEstimates is positive, we
-	// multiply it by 2 (i.e. we avoid 1). Downstream of raft, we use old
-	// behavior for ContainsEstimates=1 and the additive behavior for
-	// anything else.
-	VersionContainsEstimatesCounter
 	// VersionNamespaceTableWithSchemas is
 	// https://github.com/cockroachdb/cockroach/pull/41977
 	//
 	// It represents the migration to a new system.namespace table that has an
 	// added parentSchemaID column. In addition to the new column, the table is
 	// no longer in the system config range -- implying it is no longer gossiped.
 	VersionNamespaceTableWithSchemas
-	// VersionAuthLocalAndTrustRejectMethods introduces the HBA rule
-	// prefix 'local' and auth methods 'trust' and 'reject', for use
-	// in server.host_based_authentication.configuration.
-	//
-	// A separate cluster version ensures the new syntax is not
-	// introduced while previous-version nodes are still running, as
-	// this would block any new SQL client.
-	VersionAuthLocalAndTrustRejectMethods
 
 	// TODO(irfansharif): The versions above can/should all be removed. They
 	// were orinally introduced in v20.1. There are inflight PRs to do so
@@ -257,22 +228,10 @@ const (
 // minor version until we are absolutely sure that no new migrations will need
 // to be added (i.e., when cutting the final release candidate).
 var versionsSingleton = keyedVersions([]keyedVersion{
-	{
-		Key:     Version19_1,
-		Version: roachpb.Version{Major: 19, Minor: 1},
-	},
-	{
-		Key:     VersionContainsEstimatesCounter,
-		Version: roachpb.Version{Major: 19, Minor: 2, Internal: 2},
-	},
 	{
 		Key:     VersionNamespaceTableWithSchemas,
 		Version: roachpb.Version{Major: 19, Minor: 2, Internal: 5},
 	},
-	{
-		Key:     VersionAuthLocalAndTrustRejectMethods,
-		Version: roachpb.Version{Major: 19, Minor: 2, Internal: 8},
-	},
 
 	// v20.2 versions.
 	{

pkg/kv/kvserver/batcheval/cmd_add_sstable.go

@@ -13,7 +13,6 @@ package batcheval
 import (
 	"context"
 
-	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/batcheval/result"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverpb"
@@ -175,7 +174,6 @@ func EvalAddSSTable(
 		stats.Subtract(skippedKVStats)
 		stats.ContainsEstimates = 0
 	} else {
-		_ = clusterversion.VersionContainsEstimatesCounter // see for info on ContainsEstimates migration
 		stats.ContainsEstimates++
 	}
 

pkg/kv/kvserver/batcheval/cmd_end_transaction.go

@@ -1020,9 +1020,6 @@ func splitTriggerHelper(
 	}
 
 	deltaPostSplitLeft := h.DeltaPostSplitLeft()
-	if !rec.ClusterSettings().Version.IsActive(ctx, clusterversion.VersionContainsEstimatesCounter) {
-		deltaPostSplitLeft.ContainsEstimates = 0
-	}
 	return deltaPostSplitLeft, pd, nil
 }
 

pkg/kv/kvserver/batcheval/cmd_recompute_stats.go

@@ -13,7 +13,6 @@ package batcheval
 import (
 	"context"
 
-	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/batcheval/result"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/rditer"
@@ -102,12 +101,6 @@ func RecomputeStats(
 		// stats for timeseries ranges (which go cold and the approximate stats are
 		// wildly overcounting) and this is paced by the consistency checker, but it
 		// means some extra engine churn.
-		if !cArgs.EvalCtx.ClusterSettings().Version.IsActive(ctx, clusterversion.VersionContainsEstimatesCounter) {
-			// We are running with the older version of MVCCStats.ContainsEstimates
-			// which was a boolean, so we should keep it in {0,1} and not reset it
-			// to avoid racing with another command that sets it to true.
-			delta.ContainsEstimates = currentStats.ContainsEstimates
-		}
 		cArgs.Stats.Add(delta)
 	}
 

pkg/kv/kvserver/replica_application_state_machine.go

@@ -15,7 +15,6 @@ import (
 	"fmt"
 	"time"
 
-	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/apply"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverbase"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverpb"
@@ -796,46 +795,12 @@ func (b *replicaAppBatch) stageTrivialReplicatedEvalResult(
 	}
 	res := cmd.replicatedResult()
 
-	// Detect whether the incoming stats contain estimates that resulted from the
-	// evaluation of a command under the 19.1 cluster version. These were either
-	// evaluated on a 19.1 node (where ContainsEstimates is a bool, which maps
-	// to 0 and 1 in 19.2+) or on a 19.2 node which hadn't yet had its cluster
-	// version bumped.
-	//
-	// 19.2 nodes will never emit a ContainsEstimates outside of 0 or 1 until
-	// the cluster version is active (during command evaluation). When the
-	// version is active, they will never emit odd positive numbers (1, 3, ...).
-	//
-	// As a result, we can pinpoint exactly when the proposer of this command
-	// has used the old cluster version: it's when the incoming
-	// ContainsEstimates is 1. If so, we need to assume that an old node is processing
-	// the same commands (as `true + true = true`), so make sure that `1 + 1 = 1`.
-	_ = clusterversion.VersionContainsEstimatesCounter // see for info on ContainsEstimates migration
-	deltaStats := res.Delta.ToStats()
-	if deltaStats.ContainsEstimates == 1 && b.state.Stats.ContainsEstimates == 1 {
-		deltaStats.ContainsEstimates = 0
-	}
-
 	// Special-cased MVCC stats handling to exploit commutativity of stats delta
 	// upgrades. Thanks to commutativity, the spanlatch manager does not have to
 	// serialize on the stats key.
+	deltaStats := res.Delta.ToStats()
 	b.state.Stats.Add(deltaStats)
-	// Exploit the fact that a split will result in a full stats
-	// recomputation to reset the ContainsEstimates flag.
-	// If we were running the new VersionContainsEstimatesCounter cluster version,
-	// the consistency checker will be able to reset the stats itself, and splits
-	// will as a side effect also remove estimates from both the resulting left and right hand sides.
-	//
-	// TODO(tbg): this can be removed in v20.2 and not earlier.
-	// Consider the following scenario:
-	// - all nodes are running 19.2
-	// - all nodes rebooted into 20.1
-	// - cluster version bumped, but node1 doesn't receive the gossip update for that
-	// node1 runs a split that should emit ContainsEstimates=-1, but it clamps it to 0/1 because it
-	// doesn't know that 20.1 is active.
-	if res.Split != nil && deltaStats.ContainsEstimates == 0 {
-		b.state.Stats.ContainsEstimates = 0
-	}
+
 	if res.State != nil && res.State.UsingAppliedStateKey && !b.state.UsingAppliedStateKey {
 		b.migrateToAppliedStateKey = true
 	}

pkg/kv/kvserver/replica_proposal.go

@@ -792,26 +792,9 @@ func (r *Replica) evaluateProposal(
 		res.Replicated.Timestamp = ba.Timestamp
 		res.Replicated.Delta = ms.ToStatsDelta()
 
-		_ = clusterversion.VersionContainsEstimatesCounter // see for info on ContainsEstimates migration
-		if r.ClusterSettings().Version.IsActive(ctx, clusterversion.VersionContainsEstimatesCounter) {
-			// Encode that this command (and any that follow) uses regular arithmetic for ContainsEstimates
-			// by making sure ContainsEstimates is > 1.
-			// This will be interpreted during command application.
-			if res.Replicated.Delta.ContainsEstimates > 0 {
-				res.Replicated.Delta.ContainsEstimates *= 2
-			}
-		} else {
-			// This range may still need to have its commands processed by nodes which treat ContainsEstimates
-			// as a bool, so clamp it to {0,1}. This enables use of bool semantics in command application.
-			if res.Replicated.Delta.ContainsEstimates > 1 {
-				res.Replicated.Delta.ContainsEstimates = 1
-			} else if res.Replicated.Delta.ContainsEstimates < 0 {
-				// The caller should have checked the cluster version. At the
-				// time of writing, this is only RecomputeStats and the split
-				// trigger, which both have the check, but better safe than sorry.
-				log.Fatalf(ctx, "cannot propose negative ContainsEstimates "+
-					"without VersionContainsEstimatesCounter in %s", ba.Summary())
-			}
+		// This is the result of a migration. See the field for more details.
+		if res.Replicated.Delta.ContainsEstimates > 0 {
+			res.Replicated.Delta.ContainsEstimates *= 2
 		}
 
 		// If the cluster version doesn't track abort span size in MVCCStats, we