Skip to content

Commit

Permalink
sql: periodically clear per app transaction statistics map
Browse files Browse the repository at this point in the history
For every app, we collect both statement and transaction statistics.
This datastructure is periodically reset to ensure it doesn't get
unboundedly large. This happens by manually resetting the
datastructures inside the app stats map instead of resetting the outer
map. Recently, we started collecting transaction statistics here in
addition to statement statistics, but we never added code to reset
the transaction statistics map. This patch corrects that oversight.

Fixes cockroachdb#54452

Release note (bug fix): There was a bug in transaction statistics
collection that could let the datastructure grow unboundedly large.
This is now fixed, and the resetting happens at the same cadence as
statement statistics.
  • Loading branch information
arulajmani committed Sep 16, 2020
1 parent 27740c5 commit 668de6b
Showing 1 changed file with 5 additions and 4 deletions.
9 changes: 5 additions & 4 deletions pkg/sql/app_stats.go
Original file line number Diff line number Diff line change
Expand Up @@ -475,9 +475,9 @@ func (s *sqlStats) getStatsForApplication(appName string) *appStats {
return a
}

// resetAndMaybeDumpStats clears all the stored per-app and per-statement
// statistics. If target s not nil, then the stats in s will be flushed
// into target.
// resetAndMaybeDumpStats clears all the stored per-app, per-statement and
// per-transaction statistics. If target s not nil, then the stats in s will be
// flushed into target.
func (s *sqlStats) resetAndMaybeDumpStats(ctx context.Context, target *sqlStats) {
// Note: we do not clear the entire s.apps map here. We would need
// to do so to prevent problems with a runaway client running `SET
Expand Down Expand Up @@ -518,13 +518,14 @@ func (s *sqlStats) resetAndMaybeDumpStats(ctx context.Context, target *sqlStats)

// Only save a copy of a if we need to dump a copy of the stats.
if target != nil {
aCopy := &appStats{st: a.st, stmts: a.stmts}
aCopy := &appStats{st: a.st, stmts: a.stmts, txns: a.txns}
appStatsCopy[appName] = aCopy
}

// Clear the map, to release the memory; make the new map somewhat already
// large for the likely future workload.
a.stmts = make(map[stmtKey]*stmtStats, len(a.stmts)/2)
a.txns = make(map[txnKey]*txnStats, len(a.txns)/2)
a.Unlock()
}
s.lastReset = timeutil.Now()
Expand Down

0 comments on commit 668de6b

Please sign in to comment.