Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: do not consider TCL stmts in sql stats tracing #138041

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xinhaoz
Copy link
Member

@xinhaoz xinhaoz commented Dec 27, 2024

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. TestSampledStatsCollectionOnNewFingerprint
is also fixed as it was assuming that previously seen statements in
new transactions would trigger this sampling behaviour when it should
not - this was being erronneously triggered by BEGIN.

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

Copy link

blathers-crl bot commented Dec 27, 2024

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?

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

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@xinhaoz xinhaoz force-pushed the disable-sampling-tcl branch 3 times, most recently from b7f7b35 to 443406e Compare December 30, 2024 16:30
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. TestSampledStatsCollectionOnNewFingerprint
is also fixed as it was assuming that previously seen statements in
new transactions would trigger this sampling behaviour when it should
not - this was being erronneously triggered by `BEGIN`.

```
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: cockroachdb#133307

Release note: None
@xinhaoz xinhaoz force-pushed the disable-sampling-tcl branch from 443406e to 6da47a8 Compare December 30, 2024 16:34
@xinhaoz xinhaoz marked this pull request as ready for review December 30, 2024 16:35
@xinhaoz xinhaoz requested review from a team and kyle-a-wong and removed request for a team December 30, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants