server: SkipApproxTotalStats fails to skip MVCC collection on nodes designated for zero spans

[alyshanjahani-crl]

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, the skip decision is gated on the list being non-empty:

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), 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 server: Add SkipApproxTotalStats to SpanStatsRequest #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 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:

   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:

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: server: Add SkipApproxTotalStats to SpanStatsRequest #161819, server: modify SpanStats to skip populating ApproximateTotalStats and only fetch MVCC stats from one node #138792.

Jira issue: CRDB-66522

[blathers-crl]

Hi @alyshanjahani-crl, please add a branch-* label to identify the earliest affected branch for this C-bug

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}