cockroachdb / cockroachdb/cockroach
server: SkipApproxTotalStats fails to skip MVCC collection on nodes designated for zero spans
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
**Describe the problem**
`SkipApproxTotalStats` (added in #161819, fixing #138792) is meant to ensure that during a `SpanStats` fan-out, only **one** node per span collects logical MVCC stats. The coordinator picks a designated MVCC node per span and populates `SpansRequiringMvcc` on that node's per-node request.
The mechanism relies on a non-empty `SpansRequiringMvcc` list to signal "restrict MVCC collection to this subset." But a replica-holding node that is designated for *none* of its spans receives an **empty** `SpansRequiringMvcc`, which is indistinguishable from "field not set." Such a node falls through and computes MVCC stats for **all** of its spans — the opposite of the intended behavior.
**Root cause**
In [`getLocalStats`](https://github.com/cockroachdb/cockroach/blob/master/pkg/server/span_stats_server.go#L256-L262), the skip decision is gated on the list being non-empty:
```go
skipMvcc := req.SkipMvccStats
if !skipMvcc && len(req.SpansRequiringMvcc) > 0 { // guard requires a non-empty list
_, requiresMvcc := spansRequiringMvcc[span.String()]
skipMvcc = !requiresMvcc
}
```
On the coordinator side ([`spanStatsFanOut` → `nodeFn`](https://github.com/cockroachdb/cockroach/blob/master/pkg/server/span_stats_server.go#L143-L162)), the per-node request sets only `SpansRequiringMvcc` and never sets `SkipMvccStats`. So when a node's computed `spansRequiringMvcc` is empty, the receiving node sees `SkipMvccStats == false` and `len(SpansRequiringMvcc) == 0`, and computes MVCC for every span it holds.
This is the proto3 empty-vs-unset ambiguity: `SpansRequiringMvcc` is overloaded to mean both "which of my spans do I compute" and "am I participating in the optimization at all," and those diverge precisely when a node is designated for zero spans.
**Conditions that trigger it**
The bug affects a contacted (replica-holding) node when it is the designated MVCC node for **none** of its spans:
- **Rare when spans ≥ nodes** (every node tends to be designated for at least one span) — this is the regime the #161819 benchmark measured (TPCC schema + table metadata job, many spans / few nodes), which is why the ~3x RangeStats reduction was real and the bug went unnoticed.
- **Common when spans < nodes**, and worst-case for a **single large span whose ranges are spread across many nodes**: exactly one node is designated; every other replica-holding node gets an empty list and computes full MVCC for the whole span.
**Expected behavior**
When `SkipApproxTotalStats` is set, exactly one node per span should compute MVCC stats, and `ApproximateTotalStats` should equal `TotalStats`, regardless of the ratio of spans to nodes.
**Impact**
For a single span covering N ranges with replicas spread across M nodes:
1. **Performance:** ~M nodes each do a full-span meta scan and issue `RangeStats` RPCs to the leaseholders of every contained range, instead of 1 node. The optimization saves nothing in its adversarial regime.
2. **Correctness:** [`collectSpanStatsResponses`](https://github.com/cockroachdb/cockroach/blob/master/pkg/server/span_stats_server.go#L222-L226) sums `TotalStats` across all responding nodes into `ApproximateTotalStats`. With M nodes returning full MVCC stats, `ApproximateTotalStats ≈ M × TotalStats`. This **violates the documented invariant** in [`span_stats.proto`](https://github.com/cockroachdb/cockroach/blob/master/pkg/roachpb/span_stats.proto#L38-L43):
> When set ApproximateTotalStats will equal TotalStats since MVCC stats are collected from only one node rather than accumulated across all replicas.
i.e. the flag produces the exact overcount it was introduced to eliminate.
**Suggested fix**
Make the per-node request unambiguous. In `nodeFn`, when `SkipApproxTotalStats` is set and the computed `spansRequiringMvcc` is empty, tell the node to skip MVCC entirely rather than sending an empty list:
```go
if req.SkipApproxTotalStats {
for _, span := range nodeSpans {
if mvccNodeForSpan[span.String()] == nodeID {
spansRequiringMvcc = append(spansRequiringMvcc, span)
}
}
if len(spansRequiringMvcc) == 0 {
skipMvcc = true // designated for none of its spans; report physical bytes only
}
}
```
and pass `SkipMvccStats: skipMvcc` on the per-node request. This stays compatible with `verifySpanStatsRequest` (the two fields remain mutually exclusive — `SkipMvccStats` is only set in the empty-`spansRequiringMvcc` branch).
A cleaner long-term option is to stop overloading list-emptiness — carry the coordinator's `SkipApproxTotalStats` intent to the node explicitly so an empty designated-set unambiguously means "compute no MVCC" rather than "not participating." Either approach needs a mixed-version guard, since older nodes won't honor the new interpretation during a fan-out.
**Additional context**
- No existing test exercises `SpansRequiringMvcc` / `SkipApproxTotalStats`; a regression test covering the few-spans-many-nodes and single-large-span cases should accompany the fix.
- Related: #161819, #138792.
Jira issue: CRDB-66522
Contributor guide
Assessment
This issue has not been assessed yet.