Skip to content

server/span_stats: fix SkipApproxTotalStats empty-ambiguity opt-out [fj4WqyCCw3C5ShR1RfB7MoBPTpkRrBFYP1uT35g3MvT] - #173207

Open
waterWang wants to merge 1 commit into
cockroachdb:masterfrom
waterWang:fix-173200-skip-approx-total-stats-empty-ambiguity
Open

server/span_stats: fix SkipApproxTotalStats empty-ambiguity opt-out [fj4WqyCCw3C5ShR1RfB7MoBPTpkRrBFYP1uT35g3MvT]#173207
waterWang wants to merge 1 commit into
cockroachdb:masterfrom
waterWang:fix-173200-skip-approx-total-stats-empty-ambiguity

Conversation

@waterWang

Copy link
Copy Markdown

Fixes #173200

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" in proto3. Such a node falls through and computes MVCC stats for all of its spans — the opposite of the intended behavior.

In getLocalStats, the skip decision is gated on the list being non-empty:

skipMvcc := req.SkipMvccStats
if !skipMvcc && len(req.SpansRequiringMvcc) > 0 {
    _, requiresMvcc := spansRequiringMvcc[span.String()]
    skipMvcc = !requiresMvcc
}

Because the coordinator sets only SpansRequiringMvcc (never SkipMvccStats), a node with an empty computed list sees SkipMvccStats == false and len(SpansRequiringMvcc) == 0, and computes MVCC for every span it holds.

Fix

When SkipApproxTotalStats is set and the computed spansRequiringMvcc for a node is empty (the node is the designated MVCC collector for none of its spans), set SkipMvccStats: true on the per-node request so the receiving node skips MVCC collection entirely and reports physical bytes only.

This makes the per-node request unambiguous: SkipApproxTotalStats now guarantees exactly one node per span computes MVCC stats, and ApproximateTotalStats equals TotalStats regardless of the spans-to-nodes ratio — restoring the invariant documented in roachpb/span_stats.proto.

Test plan

  • Logic is a single gating change in the coordinator fan-out (nodeFn).

@waterWang
waterWang requested a review from a team as a code owner August 8, 2026 01:34
@waterWang
waterWang requested review from alyshanjahani-crl and removed request for a team August 8, 2026 01:34
@blathers-crl

blathers-crl Bot commented Aug 8, 2026

Copy link
Copy Markdown

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

Before a member of our team reviews your PR, I have some potential action items for you:

  • Please ensure your git commit message contains a release note.
  • When CI has completed, please ensure no errors have appeared.

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

@blathers-crl blathers-crl Bot added the O-community Originated from the community label Aug 8, 2026
@cockroachlabs-cla-agent

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

O-community Originated from the community

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant