Skip to content

Commit

Permalink
sql: do not consider TCL stmts in sql stats tracing
Browse files Browse the repository at this point in the history
For SQL stats we trace every new fingerprint in an application
container in order to populate statistics that are only available
via tracing for each fingerprint. This sampling logic worked by
tracking the SQL fingerprint strings encountered by each application
but did not factor in fingerprints that are not tracked by SQL stats,
such as TCL statements (BEGIN, COMMIT, ROLLBACK), were being treated
as new fingerprints on each execution which resulted in tracing being
turned on. This commit ensures that we don't consider TCL statements
in this sampling strategy.

```
name                                   old time/op    new time/op    delta
Sysbench/SQL/3node/oltp_read_write-10    5.61ms ± 3%    5.69ms ± 4%    ~     (p=0.167 n=8+9)

name                                   old alloc/op   new alloc/op   delta
Sysbench/SQL/3node/oltp_read_write-10    2.15MB ± 2%    2.12MB ± 2%  -1.24%  (p=0.029 n=10+10)

name                                   old allocs/op  new allocs/op  delta
Sysbench/SQL/3node/oltp_read_write-10     10.7k ± 1%     10.5k ± 0%  -1.69%  (p=0.000 n=10+9)
```

Epic: none
Part of: #133307

Release note: None
  • Loading branch information
xinhaoz committed Dec 27, 2024
1 parent 3aa4b7f commit b7f7b35
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 6 deletions.
4 changes: 2 additions & 2 deletions pkg/sql/conn_executor_exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,7 @@ func (ex *connExecutor) execStmtInOpenState(

ctx = ih.Setup(
ctx, ex.server.cfg, ex.statsCollector, p, ex.stmtDiagnosticsRecorder,
stmt.StmtNoConstants, os.ImplicitTxn.Get(),
&stmt, os.ImplicitTxn.Get(),
// This goroutine is the only one that can modify
// txnState.mu.priority, so we don't need to get a mutex here.
ex.state.mu.priority,
Expand Down Expand Up @@ -1462,7 +1462,7 @@ func (ex *connExecutor) execStmtInOpenStateWithPausablePortal(
if !portal.isPausable() || portal.pauseInfo.execStmtInOpenState.ihWrapper == nil {
ctx = ih.Setup(
ctx, ex.server.cfg, ex.statsCollector, p, ex.stmtDiagnosticsRecorder,
vars.stmt.StmtNoConstants, os.ImplicitTxn.Get(),
&vars.stmt, os.ImplicitTxn.Get(),
// This goroutine is the only one that can modify
// txnState.mu.priority, so we don't need to get a mutex here.
ex.state.mu.priority,
Expand Down
14 changes: 10 additions & 4 deletions pkg/sql/instrumentation.go
Original file line number Diff line number Diff line change
Expand Up @@ -419,12 +419,12 @@ func (ih *instrumentationHelper) Setup(
statsCollector *sslocal.StatsCollector,
p *planner,
stmtDiagnosticsRecorder *stmtdiagnostics.Registry,
fingerprint string,
stmt *Statement,
implicitTxn bool,
txnPriority roachpb.UserPriority,
collectTxnExecStats bool,
) (newCtx context.Context) {
ih.fingerprint = fingerprint
ih.fingerprint = stmt.StmtNoConstants
ih.implicitTxn = implicitTxn
ih.txnPriority = txnPriority
ih.codec = cfg.Codec
Expand All @@ -448,7 +448,7 @@ func (ih *instrumentationHelper) Setup(

default:
ih.collectBundle, ih.diagRequestID, ih.diagRequest =
stmtDiagnosticsRecorder.ShouldCollectDiagnostics(ctx, fingerprint, "" /* planGist */)
stmtDiagnosticsRecorder.ShouldCollectDiagnostics(ctx, stmt.StmtNoConstants, "" /* planGist */)
// IsRedacted will be false when ih.collectBundle is false.
ih.explainFlags.RedactValues = ih.explainFlags.RedactValues || ih.diagRequest.IsRedacted()
}
Expand All @@ -457,7 +457,7 @@ func (ih *instrumentationHelper) Setup(
ih.withStatementTrace = cfg.TestingKnobs.WithStatementTrace

var previouslySampled bool
previouslySampled, ih.savePlanForStats = statsCollector.ShouldSample(fingerprint, implicitTxn, p.SessionData().Database)
previouslySampled, ih.savePlanForStats = statsCollector.ShouldSample(stmt.StmtNoConstants, implicitTxn, p.SessionData().Database)

defer func() { ih.finalizeSetup(newCtx, cfg) }()

Expand All @@ -482,6 +482,12 @@ func (ih *instrumentationHelper) Setup(
}

shouldSampleFirstEncounter := func() bool {
if stmt.AST.StatementType() == tree.TypeTCL {
// We don't collect stats for TCL statements so
// there's no need to trace them.
return false
}

// If this is the first time we see this statement in the current stats
// container, we'll collect its execution stats anyway (unless the user
// disabled txn or stmt stats collection entirely).
Expand Down

0 comments on commit b7f7b35

Please sign in to comment.