Merge pull request #109234 from zachlite/backport23.1-108852

release-23.1: server, sql: span stats returns logical MVCC stats
cockroachdb · Aug 22, 2023 · 211fc93 · 211fc93
2 parents 662a60c + f873042
commit 211fc93
Show file tree

Hide file tree

Showing 3 changed files with 27 additions and 13 deletions.
diff --git a/pkg/roachpb/span_stats.proto b/pkg/roachpb/span_stats.proto
@@ -36,13 +36,18 @@ 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
   // key sorts after the range start, and whose end key sorts before the
   // range end, will have a range_count value of 1.
   int32 range_count = 2;
-  uint64 approximate_disk_bytes = 3;
+  // 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; It represents a physical value across all replicas.
+  // NB: The explicit jsontag prevents 'omitempty` from being added by default.
+  uint64 approximate_disk_bytes = 3 [(gogoproto.jsontag) = "approximate_disk_bytes"];
 }
 
 message SpanStatsResponse {

diff --git a/pkg/server/span_stats_server.go b/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,15 +103,22 @@ func (s *systemStatusServer) spanStatsFanOut(
 
 		nodeResponse := resp.(*roachpb.SpanStatsResponse)
 
+		// Values of ApproximateDiskBytes 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 {
+			// Accumulate physical values across all replicas:
+			res.SpanToStats[spanStr].ApproximateDiskBytes += spanStats.ApproximateDiskBytes
+
+			// Logical values: take the values from the node that responded first.
+			// TODO: This should really be read from the leaseholder.
+			if _, ok := responses[spanStr]; !ok {
+				res.SpanToStats[spanStr].TotalStats = spanStats.TotalStats
 				res.SpanToStats[spanStr].RangeCount = spanStats.RangeCount
+				responses[spanStr] = struct{}{}
 			}
-
-			res.SpanToStats[spanStr].TotalStats.Add(spanStats.TotalStats)
-			res.SpanToStats[spanStr].ApproximateDiskBytes += spanStats.ApproximateDiskBytes
 		}
 	}
 

diff --git a/pkg/server/span_stats_test.go b/pkg/server/span_stats_test.go
@@ -142,12 +142,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 {