cockroachdb / cockroachdb/cockroach
sql: statement bundle assembly is synchronous on the session, perturbing the statement it measures
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
**Is your feature request related to a problem? Please describe.**
Collecting a statement bundle costs the triggering transaction ~175ms, paid synchronously on the session. `instrumentationHelper.Finish` is a `defer` in `execStmtInOpenState`, so it runs inside the open transaction, before the connection can take the next command, and it calls `buildStatementBundle` and `bundle.insert` straight-line with no goroutine.
Most of that cost is not tracing. Measured on an idle 5-node cluster:
| | cost |
|---|---|
| tracing overhead alone (`EXPLAIN ANALYZE` vs `EXPLAIN ANALYZE (DEBUG)`, 15 point lookups) | **+4ms** flat |
| `SHOW CREATE TABLE`, per referenced table | **~19ms** |
| `SHOW STATISTICS FOR TABLE`, per referenced table | **~4.5ms** |
| full assembly for a single-table TPCC statement | **~175ms** |
The remainder is the opt/vec/distsql EXPLAIN variants, `env.sql`, `descriptors.json`, trace to Jaeger JSON, gzip and the blob insert. Tracing is ~2% of it; environment and cluster-config collection, packaging and persistence are the rest.
This contaminates the signal bundles exist to capture. Arming one fingerprint on a TPCC workload took the newOrder transaction max from 65ms to 193ms. Arming the full fingerprint set moved aggregate p99 3-7x for the tick containing it, via three multipliers:
1. **Per-node consumption** — `ShouldCollectDiagnostics` deletes the matched fingerprint from `Registry.mu.requestFingerprints`, a per-node map, so every node traces its own first match. N times the cost for one stored bundle.
2. **Stacking** — a multi-statement transaction with several armed fingerprints pays several assemblies in one transaction.
3. **Held intents** — the transaction stays open ~10x longer than normal, which will block others under contention.
So a bundle collected to explain a latency problem can be measuring delay that the collection itself introduced.
**Describe the solution you'd like**
A single `bundleCollector` goroutine per SQL server, fed what it needs to finish the job: statement fingerprint and SQL, the trace recording, rendered plan strings, referenced table names, txn timestamp, and the requesting user.
The split is fairly clean. Plan rendering has to stay on-thread — `buildStatementBundle` takes `&p.curPlan`, which is reset when the statement ends — but it is pure formatting over the memo and cheap. Everything expensive needs only names and values: catalog reads, stats, `env.sql`, `descriptors.json`, gzip, insert. Moving those should cut in-statement cost by an order of magnitude.
Design points worth settling:
- `EXPLAIN ANALYZE (DEBUG)` returns a bundle id the caller downloads immediately, so that path likely stays synchronous (and a number of tests depend on it).
- Bundle collection currently survives statement timeout by rebuilding on a background context with a 10s deadline; async makes that easier but the semantics need restating.
- A bounded queue plus a dropped-bundle metric, otherwise a burst silently loses bundles.
- The internal queries run as the requesting user, so identity travels with the message.
**Describe alternatives you've considered**
Reducing the work — fewer EXPLAIN variants, skipping `env.sql` — shrinks the constant but leaves it on the session, and the collected content is the point.
Throttling requests spreads the damage rather than removing it, and does not help the low-QPS case: at ~14 qps an op sees ~140 requests per 10s window, so p99 is rank 1 and a single bundle anywhere in the window *is* that op's p99.
**Additional context**
Code references:
- [`conn_executor_exec.go` L703](https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/conn_executor_exec.go#L703) — the `defer ih.Finish` in `execStmtInOpenState`
- [`instrumentation.go` L809](https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/instrumentation.go#L809) — `buildStatementBundle`
- [`instrumentation.go` L820](https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/instrumentation.go#L820) — `bundle.insert`
- [`statement_diagnostics.go`](https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/stmtdiagnostics/statement_diagnostics.go) — `Registry.ShouldCollectDiagnostics`, per-node request consumption
Found while investigating a recurring p99 spike in an online-restore roachtest that turned out to be the test's own bundle collection rather than product behaviour.
Jira issue: CRDB-66297
Contributor guide
Assessment
This issue has not been assessed yet.