storage: Migrate MVCCStats.contains_estimates from bool to int64

This migration allows ContainsEstimates to be reset after a stats
recomputation (by returning a -ContainsEstimates delta) without
worrying about a race condition. Another command may add 1 to it
flag and it will still be stored as valid.

Resolves cockroachdb#37120

Release note: None

docs/generated/settings/settings.html

@@ -122,6 +122,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-4</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-5</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

@@ -621,10 +621,10 @@ 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
+					ms.ContainsEstimates = 1
 					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

@@ -72,6 +72,7 @@ const (
 	VersionQueryTxnTimestamp
 	VersionStickyBit
 	VersionParallelCommits
+	VersionContainsEstimatesCounter
 
 	// Add new versions here (step one of two).
 
@@ -479,6 +480,11 @@ var versionsSingleton = keyedVersions([]keyedVersion{
 		Key:     VersionParallelCommits,
 		Version: roachpb.Version{Major: 19, Minor: 1, Unstable: 4},
 	},
+	{
+		// VersionContainsEstimatesCounter is https://github.com/cockroachdb/cockroach/pull/37583.
+		Key:     VersionContainsEstimatesCounter,
+		Version: roachpb.Version{Major: 19, Minor: 1, Unstable: 5},
+	},
 
 	// Add new versions here (step two of two).
 

pkg/sql/logictest/testdata/logic_test/builtin_function

@@ -2186,7 +2186,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: XXX_NoUnkeyedLiteral:{} XXX_sizecache:}
+1  RANGE_CONSISTENT  stats: {ContainsEstimates:0 LastUpdateNanos: IntentAge: GCBytesAge: LiveBytes: LiveCount: KeyBytes: KeyCount: ValBytes: ValCount: IntentBytes: IntentCount: SysBytes: SysCount: XXX_NoUnkeyedLiteral:{} XXX_sizecache:}
 
 # 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

@@ -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/batcheval/result"
 	"github.com/cockroachdb/cockroach/pkg/storage/engine"
 	"github.com/cockroachdb/cockroach/pkg/storage/engine/enginepb"
@@ -96,7 +97,14 @@ func EvalAddSSTable(
 	// Callers can trigger such a re-computation to fixup any discrepancies (and
 	// remove the ContainsEstimates flag) after they are done ingesting files by
 	// sending an explicit recompute.
-	stats.ContainsEstimates = true
+	//
+	// If we are running the old ContainsEstimates version (boolean), we just set it to 1.
+	// Otherwise we make it bigger than one to indicate that we are running the newer version.
+	if !cArgs.EvalCtx.ClusterSettings().Version.IsActive(cluster.VersionContainsEstimatesCounter) {
+		stats.ContainsEstimates = 1
+	} else {
+		stats.ContainsEstimates += 2
+	}
 	ms.Add(stats)
 
 	return result.Result{

pkg/storage/batcheval/cmd_add_sstable_test.go

@@ -348,6 +348,7 @@ func TestAddSSTableMVCCStats(t *testing.T) {
 	}()
 
 	cArgs := batcheval.CommandArgs{
+		EvalCtx: mockEvalCtx{},
 		Header: roachpb.Header{
 			Timestamp: hlc.Timestamp{WallTime: 7},
 		},
@@ -382,7 +383,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))
 	}

pkg/storage/batcheval/cmd_end_transaction.go

@@ -868,7 +868,7 @@ func splitTrigger(
 
 	// Compute (absolute) stats for RHS range.
 	var rightMS enginepb.MVCCStats
-	if origBothMS.ContainsEstimates || bothDeltaMS.ContainsEstimates {
+	if origBothMS.ContainsEstimates > 0 || bothDeltaMS.ContainsEstimates > 0 {
 		// Because either the original stats or the delta stats contain
 		// estimate values, we cannot perform arithmetic to determine the
 		// new range's stats. Instead, we must recompute by iterating
@@ -1047,8 +1047,8 @@ func splitTrigger(
 	// to turn it into a delta so the upstream machinery can digest it.
 	leftDeltaMS := leftMS // start with new left-hand side absolute stats
 	recStats := rec.GetMVCCStats()
-	leftDeltaMS.Subtract(recStats)        // subtract pre-split absolute stats
-	leftDeltaMS.ContainsEstimates = false // if there were any, recomputation removed them
+	leftDeltaMS.Subtract(recStats)    // subtract pre-split absolute stats
+	leftDeltaMS.ContainsEstimates = 0 // if there were any, recomputation removed them
 
 	// Perform a similar computation for the right hand side. The difference
 	// is that there isn't yet a Replica which could apply these stats, so

pkg/storage/batcheval/cmd_recompute_stats.go

@@ -97,14 +97,6 @@ func RecomputeStats(
 	delta.Subtract(currentStats)
 
 	if !dryRun {
-		// NB: this will never clear the ContainsEstimates flag. To be able to do this,
-		// we would need to guarantee that no command that sets it is in-flight in
-		// parallel with this command. This can be achieved by blocking all of the range
-		// or by using our inside knowledge that dictates that ranges which contain no
-		// timeseries writes never have the flag reset, or by making ContainsEstimates
-		// a counter (and ensuring that we're the only one subtracting at any given
-		// time).
-		//
 		// TODO(tschottdorf): do we not want to run at all if we have estimates in
 		// this range? I think we want to as this would give us much more realistic
 		// stats for timeseries ranges (which go cold and the approximate stats are

pkg/storage/consistency_queue_test.go

@@ -478,7 +478,7 @@ func TestConsistencyQueueRecomputeStats(t *testing.T) {
 		// not affected by the workload we run below and also does not influence the
 		// GC queue score.
 		ms.SysCount += sysCountGarbage
-		ms.ContainsEstimates = false
+		ms.ContainsEstimates = 0
 
 		// Overwrite with the new stats; remember that this range hasn't upreplicated,
 		// so the consistency checker won't see any replica divergence when it runs,