Skip to content

Commit 5179c44

Browse files
dhartuniandhartunian
committed
sqlstats: fix data race between collector and ingester
I've admittedly not been able to reproduce the data race observed in the same `*sqlstats.RecordedStmtStats` object in two different locations. One of these is now redunant. The transactionID set in the `StatsCollector` happens before the data is processed by the `SQLStatsIngester` so the setting of the transactionID in the ingester is removed. If it happens to get statements without corresponding transactionIDs, that should be a bug. The reason this was introduced was likely #141767 which unified the types used by the two components. Previously, they were different structs and ownership was clear and required editing the transactionID twice. This PR also introduces a `doc.go` file to the `sslocal` package in an effort to guide the reviewer and provide context for future work. The diagram reflects the changes in this commit. Resolves: #146796 Release note: None
1 parent f42beff commit 5179c44

File tree

4 files changed

+95
-9
lines changed

4 files changed

+95
-9
lines changed

‎pkg/sql/sqlstats/sslocal/BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ go_library(
44
name = "sslocal",
55
srcs = [
66
"cluster_settings.go",
7+
"doc.go",
78
"sql_stats.go",
89
"sql_stats_ingestor.go",
910
"sslocal_iterator.go",

‎pkg/sql/sqlstats/sslocal/doc.go‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
// Copyright 2025 The Cockroach Authors.
2+
//
3+
// Use of this software is governed by the CockroachDB Software License
4+
// included in the /LICENSE file.
5+
6+
/*
7+
This package is used to collect SQL Activity Statistics from the SQL subsystem.
8+
9+
# The recording of SQL Activity is managed through the following components:
10+
11+
1. Conn Executor: SQL Execution is managed here and stats are collected and sent
12+
to the Stats Collector.
13+
2. (sslocal) Stats Collector: Receives stats from the Conn Executor and buffers them into
14+
the `ssmemstorage.Container`
15+
3. (sslocal) SQL Stats Ingester: Receives stats from the Stats Collector and
16+
flushes them to the registry.
17+
4. Container: A per-application container that holds stats for a given
18+
application. The Ingester flushes
19+
20+
# Sequence of Operations
21+
22+
The diagram below illustrates how stats flow through the StatsCollector and
23+
StatsIngester as a transaction executes.
24+
25+
+---------------+ +-----------------+ +---------------+ +-----------+
26+
| ConnExecutor | | StatsCollector | | StatsIngester | | Container |
27+
+---------------+ +-----------------+ +---------------+ +-----------+
28+
| | | |
29+
| RecordStatement | | |
30+
|--------------------------->| | |
31+
| | -------------------------------------------\ | |
32+
| |-| *RecordedStmtStats accumulates in buffer | | |
33+
| | |------------------------------------------| | |
34+
| | | |
35+
| EndTransaction | | |
36+
|--------------------------->| | |
37+
| | ------------------------------------------\ | |
38+
| |-| set TransactionID on *RecordedStmtStats | | |
39+
| | |-----------------------------------------| | |
40+
| | | |
41+
| | IngestStatement | |
42+
| |--------------------------------------------------->| |
43+
| | | -------------------------------------------\ |
44+
| | |-| *RecordedStmtStats accumulates in buffer | |
45+
| | | |------------------------------------------| |
46+
| | | |
47+
| | RecordStatement | |
48+
| |---------------------------------------------------------------------------------------------------------->|
49+
| | | |
50+
| RecordTransaction | | |
51+
|--------------------------->| | |
52+
| | | |
53+
| | IngestTransaction | |
54+
| |--------------------------------------------------->| |
55+
| | | |
56+
| | RecordTransaction | |
57+
| |---------------------------------------------------------------------------------------------------------->|
58+
| | | -----------------------------------------------\ |
59+
| | |-| all *RecordedStmtStats in buffer are flushed | |
60+
| | | |----------------------------------------------| |
61+
| | | | ----------------------------------------\
62+
| | | |-| eventually flushed to persisted stats |
63+
| | | | |---------------------------------------|
64+
| | | |
65+
*/
66+
package sslocal
67+
68+
// Input to sequence diagram generator:
69+
/*
70+
object ConnExecutor StatsCollector StatsIngester Container
71+
ConnExecutor -> StatsCollector: RecordStatement
72+
note right of StatsCollector: *RecordedStmtStats accumulates in buffer
73+
ConnExecutor -> StatsCollector: EndTransaction
74+
note right of StatsCollector: set TransactionID on *RecordedStmtStats
75+
StatsCollector -> StatsIngester: IngestStatement
76+
note right of StatsIngester: *RecordedStmtStats accumulates in buffer
77+
StatsCollector -> Container: RecordStatement
78+
ConnExecutor -> StatsCollector: RecordTransaction
79+
StatsCollector -> StatsIngester: IngestTransaction
80+
StatsCollector -> Container: RecordTransaction
81+
note right of StatsIngester: all *RecordedStmtStats in buffer are flushed
82+
note right of Container: eventually flushed to persisted stats
83+
*/

‎pkg/sql/sqlstats/sslocal/sql_stats_ingestor.go‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -315,11 +315,6 @@ func (i *SQLStatsIngester) flushBuffer(
315315
return
316316
}
317317

318-
// Set the transaction fingerprint ID for each statement.
319-
for _, s := range *statements {
320-
s.TransactionFingerprintID = transaction.FingerprintID
321-
}
322-
323318
for _, sink := range i.sinks {
324319
sink.ObserveTransaction(ctx, transaction, *statements)
325320
}

‎pkg/sql/sqlstats/sslocal/sslocal_stats_collector.go‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,14 @@ func (s *StatsCollector) Reset(appStats *ssmemstorage.Container, phaseTime *sess
145145
// any memory allocated by underlying sql stats systems for the session
146146
// that owns this stats collector.
147147
func (s *StatsCollector) Close(_ctx context.Context, sessionID clusterunique.ID) {
148+
if s.statsIngester != nil {
149+
for _, stmt := range s.stmtBuf {
150+
stmt.TransactionFingerprintID = appstatspb.InvalidTransactionFingerprintID
151+
if s.sendInsights {
152+
s.statsIngester.IngestStatement(stmt)
153+
}
154+
}
155+
}
148156
s.stmtBuf = nil
149157
if s.statsIngester != nil {
150158
s.statsIngester.ClearSession(sessionID)
@@ -174,6 +182,9 @@ func (s *StatsCollector) EndTransaction(
174182

175183
for _, stmt := range s.stmtBuf {
176184
stmt.TransactionFingerprintID = transactionFingerprintID
185+
if s.sendInsights && s.statsIngester != nil {
186+
s.statsIngester.IngestStatement(stmt)
187+
}
177188
if err := s.flushTarget.RecordStatement(ctx, stmt); err != nil {
178189
discardedStats++
179190
}
@@ -217,10 +228,6 @@ func (s *StatsCollector) shouldObserveInsights() bool {
217228
func (s *StatsCollector) RecordStatement(
218229
ctx context.Context, value *sqlstats.RecordedStmtStats,
219230
) error {
220-
if s.sendInsights && s.statsIngester != nil {
221-
s.statsIngester.IngestStatement(value)
222-
}
223-
224231
// TODO(xinhaoz): This isn't the best place to set this, but we'll clean this up
225232
// when we refactor the stats collection code to send the stats to an ingester.
226233
s.stmtFingerprintID = value.FingerprintID

0 commit comments

Comments
 (0)