storage: Migrate MVCCStats contains_estimates from bool to int64

This migration makes ContainsEstimates a counter so that the
consistency checker can reset it (by returning a -ContainsEstimates)
delta) without racing with another command that introduces new
estimate stats.

Resolves cockroachdb#37120

Release note: None

docs/generated/settings/settings.html

@@ -129,6 +129,6 @@
 <tr><td><code>trace.debug.enable</code></td><td>boolean</td><td><code>false</code></td><td>if set, traces for recent requests can be seen in the /debug page</td></tr>
 <tr><td><code>trace.lightstep.token</code></td><td>string</td><td><code></code></td><td>if set, traces go to Lightstep using this token</td></tr>
 <tr><td><code>trace.zipkin.collector</code></td><td>string</td><td><code></code></td><td>if set, traces go to the given Zipkin instance (example: '127.0.0.1:9411'); ignored if trace.lightstep.token is set</td></tr>
-<tr><td><code>version</code></td><td>custom validation</td><td><code>19.1-8</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
+<tr><td><code>version</code></td><td>custom validation</td><td><code>19.1-9</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
 </tbody>
 </table>

pkg/ccl/storageccl/engineccl/rocksdb.go

@@ -138,7 +138,7 @@ func cStatsToGoStats(stats C.MVCCStatsResult, nowNanos int64) (enginepb.MVCCStat
 	if err := statusToError(stats.status); err != nil {
 		return ms, err
 	}
-	ms.ContainsEstimates = false
+	ms.ContainsEstimates = 0
 	ms.LiveBytes = int64(stats.live_bytes)
 	ms.KeyBytes = int64(stats.key_bytes)
 	ms.ValBytes = int64(stats.val_bytes)

pkg/cli/debug.go

@@ -616,10 +616,9 @@ func runDebugCheckStoreDescriptors(ctx context.Context, db *engine.RocksDB) erro
 
 			if !ms.Equal(claimedMS) {
 				var prefix string
-				if !claimedMS.ContainsEstimates {
+				if claimedMS.ContainsEstimates == 0 {
 					failed = true
 				} else {
-					ms.ContainsEstimates = true
 					prefix = "(ignored) "
 				}
 				fmt.Printf("\n%s%+v: diff(actual, claimed): %s\n", prefix, desc, strings.Join(pretty.Diff(ms, claimedMS), "\n"))

pkg/settings/cluster/cockroach_versions.go

@@ -43,6 +43,7 @@ const (
 	VersionLearnerReplicas
 	VersionTopLevelForeignKeys
 	VersionAtomicChangeReplicasTrigger
+	VersionContainsEstimatesCounter
 
 	// Add new versions here (step one of two).
 
@@ -520,6 +521,34 @@ var versionsSingleton = keyedVersions([]keyedVersion{
 		Key:     VersionAtomicChangeReplicasTrigger,
 		Version: roachpb.Version{Major: 19, Minor: 1, Unstable: 8},
 	},
+	{
+		// 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 commands that set ContainsEstimates must set/increase it by 1.
+		// During command evaluation, proposer nodes check the cluster version and
+		// adjust ContainsEstimates accordingly:
+		// - If the new cluster version is not active, ContainsEstimates must be kept
+		// translatable to bool and is truncated to {0,1}.
+		// - If the new cluster version is active, ContainsEstimates is multiplied by 2.
+		// Such an even positive number signals the code downstream of Raft that the
+		// new cluster version is active and all followers can use regular math to add
+		// ContainsEstimates as an integer.
+		//
+		// During command application, commands proposed under truncation are detected and
+		// the replicas' ContainsEstimates are truncated to {0,1} accordingly. This makes the
+		// new code compatible with the old code when adding/subtracting ContainsEstimates:
+		// (true + true = true) and (1 + 1 = 1), keeping ContainsEstimates <= 1 while the old
+		// cluster version is active.
+		// When the new cluster version is active, ContainsEstimates>=2 will enter Raft and
+		// add/sub will be calculated normally. This will allow commands to try to reset
+		// it without clashing with other operations, by emitting a -ContainsEstimates delta.
+		Key:     VersionContainsEstimatesCounter,
+		Version: roachpb.Version{Major: 19, Minor: 1, Unstable: 9},
+	},
 
 	// Add new versions here (step two of two).
 

pkg/sql/logictest/testdata/logic_test/builtin_function

@@ -2213,7 +2213,7 @@ SELECT crdb_internal.check_consistency(true, '\x03', '\x02')
 query ITT
 SELECT range_id, status, regexp_replace(detail, '[0-9]+', '', 'g') FROM crdb_internal.check_consistency(true, '\x02', '\xffff') WHERE range_id = 1
 ----
-1  RANGE_CONSISTENT  stats: {ContainsEstimates:false LastUpdateNanos: IntentAge: GCBytesAge: LiveBytes: LiveCount: KeyBytes: KeyCount: ValBytes: ValCount: IntentBytes: IntentCount: SysBytes: SysCount:}
+1  RANGE_CONSISTENT  stats: {ContainsEstimates:0 LastUpdateNanos: IntentAge: GCBytesAge: LiveBytes: LiveCount: KeyBytes: KeyCount: ValBytes: ValCount: IntentBytes: IntentCount: SysBytes: SysCount:}
 
 # Without explicit keys, scans all ranges (we don't test this too precisely to
 # avoid flaking the test when the range count changes, just want to know that

pkg/storage/batcheval/cmd_add_sstable.go

@@ -15,6 +15,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/storage/batcheval/result"
 	"github.com/cockroachdb/cockroach/pkg/storage/engine"
 	"github.com/cockroachdb/cockroach/pkg/storage/engine/enginepb"
@@ -165,7 +166,8 @@ func EvalAddSSTable(
 	// compute the stats for these "skipped" KVs on-the-fly while checking for the
 	// collision condition and returning their stats. The final stats would then
 	// be ms + stats - skipped_stats, and this would be accurate.
-	stats.ContainsEstimates = true
+	_ = cluster.VersionContainsEstimatesCounter // see for info on ContainsEstimates migration
+	stats.ContainsEstimates++
 	ms.Add(stats)
 
 	return result.Result{

pkg/storage/batcheval/cmd_add_sstable_test.go

@@ -378,7 +378,7 @@ func TestAddSSTableMVCCStats(t *testing.T) {
 		return afterStats
 	}()
 	evaledStats.Add(delta)
-	evaledStats.ContainsEstimates = false
+	evaledStats.ContainsEstimates = 0
 	if !afterStats.Equal(evaledStats) {
 		t.Errorf("mvcc stats mismatch: diff(expected, actual): %s", pretty.Diff(afterStats, evaledStats))
 	}
@@ -398,7 +398,7 @@ func TestAddSSTableMVCCStats(t *testing.T) {
 	if _, err := batcheval.EvalAddSSTable(ctx, e, cArgsWithStats, nil); err != nil {
 		t.Fatalf("%+v", err)
 	}
-	expected := enginepb.MVCCStats{ContainsEstimates: true, KeyCount: 10}
+	expected := enginepb.MVCCStats{ContainsEstimates: 1, KeyCount: 10}
 	if got := *cArgsWithStats.Stats; got != expected {
 		t.Fatalf("expected %v got %v", expected, got)
 	}

pkg/storage/batcheval/cmd_end_transaction.go

@@ -19,6 +19,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/storage/abortspan"
 	"github.com/cockroachdb/cockroach/pkg/storage/batcheval/result"
 	"github.com/cockroachdb/cockroach/pkg/storage/engine"
@@ -1022,14 +1023,10 @@ func splitTriggerHelper(
 		RHSDelta: *h.AbsPostSplitRight(),
 	}
 
-	// HACK(tbg): ContainsEstimates isn't an additive group (there isn't a
-	// -true), and instead of "-true" we'll emit a "true". This will all be
-	// fixed when #37583 lands (and the version is active). For now hard-code
-	// false and there's also code below Raft that interprets this (coming from
-	// a split) as a signal to reset the ContainsEstimates field to false (see
-	// applyRaftCommand).
 	deltaPostSplitLeft := h.DeltaPostSplitLeft()
-	deltaPostSplitLeft.ContainsEstimates = false
+	if !rec.ClusterSettings().Version.IsActive(cluster.VersionContainsEstimatesCounter) {
+		deltaPostSplitLeft.ContainsEstimates = 0
+	}
 	return deltaPostSplitLeft, pd, nil
 }