server, sql: span stats returns logical MVCC stats

Previously, the MVCC stats returned by span stats were mistakenly accumulated
from each node. This commit fixes that, and adds clarifying comments.

Resolves: cockroachdb#108766
Epic: CRDB-30635
Release note (bug fix): A bug was fixed where a SpanStatsRequest
would return post-replicated MVCC stats. Now, a SpanStatsRequest
returns the logical MVCC stats for the requested span.

pkg/roachpb/span_stats.go

@@ -66,10 +66,3 @@ var RangeDescPageSize = settings.RegisterIntSetting(
 		return nil
 	},
 )
-
-func (m *SpanStats) Add(other *SpanStats) {
-	m.TotalStats.Add(other.TotalStats)
-	m.ApproximateDiskBytes += other.ApproximateDiskBytes
-	m.RemoteFileBytes += other.RemoteFileBytes
-	m.ExternalFileBytes += other.ExternalFileBytes
-}

pkg/roachpb/span_stats.proto

@@ -36,6 +36,7 @@ message SpanStatsRequest {
 }
 
 message SpanStats {
+  // TotalStats are the logical MVCC stats for the requested span.
   cockroach.storage.enginepb.MVCCStats total_stats = 1 [(gogoproto.nullable) = false];
   // range_count measures the number of ranges that the request span falls within.
   // A SpanStatsResponse for a span that lies within a range, and whose start
@@ -46,15 +47,18 @@ message SpanStats {
   // ApproximateDiskBytes is the approximate size "on-disk" in all files of the
   // data in the span. NB; this *includes* files stored remotely, not just on
   // _local_ disk; see the RemoteFileBytes field below.
+  // It represents a physical value is accumulated by the fan-out.
   // NB: The explicit jsontag prevents 'omitempty` from being added by default.
   uint64 approximate_disk_bytes = 3 [(gogoproto.jsontag) = "approximate_disk_bytes"];
 
   // RemoteFileBytes is the subset of ApproximateDiskBytes which are stored in
-  // "remote" files (i.e. shared files and external files).
+  // "remote" files (i.e. shared files and external files). It represents a
+  // physical value, and is accumulated by a fan-out.
   uint64 remote_file_bytes = 5;
 
   // ExternalFileBytes is the subset of RemoteFileBytes that are in "external"
   // files (not written/owned by this cluster, such as in restored backups).
+  // It represents a physical value, and is accumulated by a fan-out.
   uint64 external_file_bytes = 6;
 
   // NEXT ID: 7.

pkg/server/span_stats_server.go

@@ -45,6 +45,8 @@ func (s *systemStatusServer) spanStatsFanOut(
 		res.SpanToStats[sp.String()] = &roachpb.SpanStats{}
 	}
 
+	responses := make(map[string]struct{})
+
 	spansPerNode, err := s.getSpansPerNode(ctx, req)
 	if err != nil {
 		return nil, err
@@ -101,13 +103,26 @@ func (s *systemStatusServer) spanStatsFanOut(
 
 		nodeResponse := resp.(*roachpb.SpanStatsResponse)
 
+		// Values of ApproximateDiskBytes, RemoteFileBytes, and ExternalFileBytes should be physical values, but
+		// TotalStats (MVCC stats) should be the logical, pre-replicated value.
+		// Note: This implementation can return arbitrarily stale values, because instead of getting
+		// MVCC stats from the leaseholder, MVCC stats are taken from the node that responded first.
+		// See #108779.
 		for spanStr, spanStats := range nodeResponse.SpanToStats {
-			// We are not counting replicas, so only consider range count
-			// if it has not been set.
-			if res.SpanToStats[spanStr].RangeCount == 0 {
+			if _, ok := responses[spanStr]; ok {
+				// Accumulate values for physical quantities:
+				res.SpanToStats[spanStr].ApproximateDiskBytes += spanStats.ApproximateDiskBytes
+				res.SpanToStats[spanStr].RemoteFileBytes += spanStats.RemoteFileBytes
+				res.SpanToStats[spanStr].ExternalFileBytes += spanStats.ExternalFileBytes
+			} else {
+				// Logical values: take the values from the node that responded first.
+				res.SpanToStats[spanStr].TotalStats = spanStats.TotalStats
 				res.SpanToStats[spanStr].RangeCount = spanStats.RangeCount
+				res.SpanToStats[spanStr].ApproximateDiskBytes = spanStats.ApproximateDiskBytes
+				res.SpanToStats[spanStr].RemoteFileBytes = spanStats.RemoteFileBytes
+				res.SpanToStats[spanStr].ExternalFileBytes = spanStats.ExternalFileBytes
+				responses[spanStr] = struct{}{}
 			}
-			res.SpanToStats[spanStr].Add(spanStats)
 		}
 	}
 

pkg/server/span_stats_test.go

@@ -143,12 +143,12 @@ func TestSpanStatsFanOut(t *testing.T) {
 	}
 
 	testCases := []testCase{
-		{spans[0], 4, int64(numNodes * 6)},
-		{spans[1], 1, int64(numNodes * 3)},
-		{spans[2], 2, int64(numNodes * 5)},
-		{spans[3], 2, int64(numNodes * 1)},
-		{spans[4], 2, int64(numNodes * 3)},
-		{spans[5], 1, int64(numNodes * 2)},
+		{spans[0], 4, int64(6)},
+		{spans[1], 1, int64(3)},
+		{spans[2], 2, int64(5)},
+		{spans[3], 2, int64(1)},
+		{spans[4], 2, int64(3)},
+		{spans[5], 1, int64(2)},
 	}
 
 	testutils.SucceedsSoon(t, func() error {