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 #37120

Release note: None

docs/generated/settings/settings.html

@@ -133,6 +133,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.2-1</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.2-2</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_check_store.go

@@ -103,7 +103,7 @@ func (cr *checkResult) Error() error {
 	if cr.err != nil {
 		errs = append(errs, cr.err.Error())
 	}
-	if !cr.actMS.Equal(enginepb.MVCCStats{}) && !cr.actMS.Equal(cr.claimMS) && !cr.claimMS.ContainsEstimates {
+	if !cr.actMS.Equal(enginepb.MVCCStats{}) && !cr.actMS.Equal(cr.claimMS) && cr.claimMS.ContainsEstimates <= 0 {
 		err := fmt.Sprintf("stats inconsistency:\n- stored:\n%+v\n- recomputed:\n%+v\n- diff:\n%s",
 			cr.claimMS, cr.actMS, strings.Join(pretty.Diff(cr.claimMS, cr.actMS), ","),
 		)
@@ -192,7 +192,7 @@ func checkStoreRangeStats(
 			errS := err.Error()
 			println(errS)
 		} else {
-			if res.claimMS.ContainsEstimates {
+			if res.claimMS.ContainsEstimates > 0 {
 				cE++
 			}
 			total.Add(res.actMS)

pkg/cli/debug_check_store_test.go

@@ -92,7 +92,7 @@ func TestDebugCheckStore(t *testing.T) {
 		sl := stateloader.Make(1)
 		ms, err := sl.LoadMVCCStats(ctx, eng)
 		require.NoError(t, err)
-		ms.ContainsEstimates = false
+		ms.ContainsEstimates = 0
 		ms.LiveBytes++
 		require.NoError(t, sl.SetMVCCStats(ctx, eng, &ms))
 	}()

pkg/settings/cluster/cockroach_versions.go

@@ -46,6 +46,7 @@ const (
 	VersionPartitionedBackup
 	Version19_2
 	VersionStart20_1
+	VersionContainsEstimatesCounter
 
 	// Add new versions here (step one of two).
 
@@ -265,6 +266,25 @@ var versionsSingleton = keyedVersions([]keyedVersion{
 		Key:     VersionStart20_1,
 		Version: roachpb.Version{Major: 19, Minor: 2, Unstable: 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, Unstable: 2},
+	},
 
 	// Add new versions here (step two of two).
 

pkg/sql/logictest/testdata/logic_test/builtin_function

@@ -2223,7 +2223,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: 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"
@@ -157,7 +158,7 @@ func EvalAddSSTable(
 	// sending an explicit recompute.
 	//
 	// There is a significant performance win to be achieved by ensuring that the
-	// stats computed are not estimates as it prevents recompuation on splits.
+	// stats computed are not estimates as it prevents recomputation on splits.
 	// Running AddSSTable with disallowShadowing=true gets us close to this as we
 	// do not allow colliding keys to be ingested. However, in the situation that
 	// two SSTs have KV(s) which "perfectly" shadow an existing key (equal ts and
@@ -170,8 +171,12 @@ func EvalAddSSTable(
 	// cumulative for this command. These stats can then be marked as accurate.
 	if args.DisallowShadowing {
 		stats.Subtract(skippedKVStats)
+		stats.ContainsEstimates = 0
+	} else {
+		_ = cluster.VersionContainsEstimatesCounter // see for info on ContainsEstimates migration
+		stats.ContainsEstimates++
 	}
-	stats.ContainsEstimates = !args.DisallowShadowing
+
 	ms.Add(stats)
 
 	if args.IngestAsWrites {

pkg/storage/batcheval/cmd_add_sstable_test.go

@@ -455,7 +455,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))
 			}
@@ -475,7 +475,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)
 			}