clusterversion,*: remove VersionContainsEstimatesCounter

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 cockroachdb#47447. Fixes cockroachdb#56401.

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

Release note: None

pkg/clusterversion/.gitattributes

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

pkg/clusterversion/cockroach_versions.go

@@ -66,7 +66,6 @@ type VersionKey int
 const (
 	_ VersionKey = iota - 1 // want first named one to start at zero
 	Version19_1
-	VersionContainsEstimatesCounter
 	VersionNamespaceTableWithSchemas
 	VersionAuthLocalAndTrustRejectMethods
 
@@ -122,25 +121,6 @@ var versionsSingleton = keyedVersions([]keyedVersion{
 		Key:     Version19_1,
 		Version: roachpb.Version{Major: 19, Minor: 1},
 	},
-	{
-		// 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.
-		Key:     VersionContainsEstimatesCounter,
-		Version: roachpb.Version{Major: 19, Minor: 2, Internal: 2},
-	},
 	{
 		// VersionNamespaceTableWithSchemas is https://github.com/cockroachdb/cockroach/pull/41977
 		//

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,11 @@ 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())
-			}
+		// 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
 		}
 
 		// If the cluster version doesn't track abort span size in MVCCStats, we

pkg/kv/kvserver/replica_test.go

@@ -34,7 +34,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/gossip"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv"
-	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/apply"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/batcheval"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/batcheval/result"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency"
@@ -12730,16 +12729,12 @@ func TestReplicateQueueProcessOne(t *testing.T) {
 }
 
 // TestContainsEstimatesClamp tests the massaging of ContainsEstimates
-// before proposing a raft command.
-// - If the proposing node's version is lower than the VersionContainsEstimatesCounter,
-// ContainsEstimates must be clamped to {0,1}.
-// - Otherwise, it should always be >1 and an even number.
+// before proposing a raft command. It should always be >1 and an even number.
+// See the comment on ContainEstimates to understand why.
 func TestContainsEstimatesClampProposal(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	_ = clusterversion.VersionContainsEstimatesCounter // see for details on the ContainsEstimates migration
-
 	someRequestToProposal := func(tc *testContext, ctx context.Context) *ProposalData {
 		cmdIDKey := kvserverbase.CmdIDKey("some-cmdid-key")
 		var ba roachpb.BatchRequest
@@ -12757,23 +12752,6 @@ func TestContainsEstimatesClampProposal(t *testing.T) {
 	// any number >1.
 	defer setMockPutWithEstimates(2)()
 
-	t.Run("Pre-VersionContainsEstimatesCounter", func(t *testing.T) {
-		ctx := context.Background()
-		stopper := stop.NewStopper()
-		defer stopper.Stop(ctx)
-		cfg := TestStoreConfig(nil)
-		version := clusterversion.VersionByKey(clusterversion.VersionContainsEstimatesCounter - 1)
-		cfg.Settings = cluster.MakeTestingClusterSettingsWithVersions(version, version, false /* initializeVersion */)
-		var tc testContext
-		tc.StartWithStoreConfigAndVersion(t, stopper, cfg, version)
-
-		proposal := someRequestToProposal(&tc, ctx)
-
-		if proposal.command.ReplicatedEvalResult.Delta.ContainsEstimates != 1 {
-			t.Error("Expected ContainsEstimates to be 1, was", proposal.command.ReplicatedEvalResult.Delta.ContainsEstimates)
-		}
-	})
-
 	t.Run("VersionContainsEstimatesCounter", func(t *testing.T) {
 		ctx := context.Background()
 		stopper := stop.NewStopper()
@@ -12790,77 +12768,6 @@ func TestContainsEstimatesClampProposal(t *testing.T) {
 
 }
 
-// TestContainsEstimatesClampApplication tests that if the ContainsEstimates
-// delta from a proposed command is 1 (and the replica state ContainsEstimates <= 1),
-// ContainsEstimates will be kept 1 in the replica state. This is because
-// ContainsEstimates==1 in a proposed command means that the proposer may run
-// with a version older than VersionContainsEstimatesCounter, in which ContainsEstimates
-// is a bool.
-func TestContainsEstimatesClampApplication(t *testing.T) {
-	defer leaktest.AfterTest(t)()
-	defer log.Scope(t).Close(t)
-
-	_ = clusterversion.VersionContainsEstimatesCounter // see for details on the ContainsEstimates migration
-
-	ctx := context.Background()
-	stopper := stop.NewStopper()
-	defer stopper.Stop(ctx)
-	tc := testContext{}
-	tc.Start(t, stopper)
-
-	// We will stage and apply 2 batches with a command that has ContainsEstimates=1
-	// and expect that ReplicaState.Stats.ContainsEstimates will not become greater than 1.
-	applyBatch := func() {
-		tc.repl.raftMu.Lock()
-		defer tc.repl.raftMu.Unlock()
-		sm := tc.repl.getStateMachine()
-		batch := sm.NewBatch(false /* ephemeral */)
-		rAppbatch := batch.(*replicaAppBatch)
-
-		lease, _ := tc.repl.GetLease()
-
-		cmd := replicatedCmd{
-			ctx: ctx,
-			ent: &raftpb.Entry{
-				// Term:  1,
-				Index: rAppbatch.state.RaftAppliedIndex + 1,
-				Type:  raftpb.EntryNormal,
-			},
-			decodedRaftEntry: decodedRaftEntry{
-				idKey: makeIDKey(),
-				raftCmd: kvserverpb.RaftCommand{
-					ProposerLeaseSequence: rAppbatch.state.Lease.Sequence,
-					ReplicatedEvalResult: kvserverpb.ReplicatedEvalResult{
-						Timestamp:      tc.Clock().Now(),
-						IsLeaseRequest: true,
-						State: &kvserverpb.ReplicaState{
-							Lease: &lease,
-						},
-						Delta: enginepb.MVCCStatsDelta{
-							ContainsEstimates: 1,
-						},
-					},
-				},
-			},
-		}
-
-		_, err := rAppbatch.Stage(apply.Command(&cmd))
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		if err := batch.ApplyToStateMachine(ctx); err != nil {
-			t.Fatal(err)
-		}
-	}
-
-	applyBatch()
-	assert.Equal(t, int64(1), tc.repl.State().ReplicaState.Stats.ContainsEstimates)
-
-	applyBatch()
-	assert.Equal(t, int64(1), tc.repl.State().ReplicaState.Stats.ContainsEstimates)
-}
-
 // setMockPutWithEstimates mocks the Put command (could be any) to simulate a command
 // that touches ContainsEstimates, in order to test request proposal behavior.
 func setMockPutWithEstimates(containsEstimatesDelta int64) (undo func()) {

pkg/storage/BUILD.bazel

@@ -74,7 +74,6 @@ go_library(
     visibility = ["//visibility:public"],
     deps = [
         "//pkg/base",
-        "//pkg/clusterversion",
         "//pkg/keys",
         "//pkg/kv/kvserver/concurrency/lock",
         "//pkg/kv/kvserver/diskmap",