-
Notifications
You must be signed in to change notification settings - Fork 4k
sql: Record statement stats for statements executed in UDFs / SPs #156905
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
base: master
Are you sure you want to change the base?
Conversation
|
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. |
🔴 Sysbench [SQL, 3node, oltp_read_write]
Reproducebenchdiff binaries: mkdir -p benchdiff/90c89be/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/90c89be2b2b315abae3bf254c0aeb79a9e67b0f6/bin/pkg_sql_tests benchdiff/90c89be/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/90c89be/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/d669516/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/d6695166a4ec91179d1e48cf31c3d821cac6d955/bin/pkg_sql_tests benchdiff/d669516/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/d669516/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: benchdiff --run=^BenchmarkSysbench/SQL/3node/oltp_read_write$ --old=d669516 --new=90c89be ./pkg/sql/tests⚪ Sysbench [KV, 3node, oltp_read_only]
Reproducebenchdiff binaries: mkdir -p benchdiff/90c89be/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/90c89be2b2b315abae3bf254c0aeb79a9e67b0f6/bin/pkg_sql_tests benchdiff/90c89be/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/90c89be/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/d669516/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/d6695166a4ec91179d1e48cf31c3d821cac6d955/bin/pkg_sql_tests benchdiff/d669516/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/d669516/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_read_only$ --old=d669516 --new=90c89be ./pkg/sql/tests⚪ Sysbench [KV, 3node, oltp_write_only]
Reproducebenchdiff binaries: mkdir -p benchdiff/90c89be/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/90c89be2b2b315abae3bf254c0aeb79a9e67b0f6/bin/pkg_sql_tests benchdiff/90c89be/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/90c89be/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/d669516/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/d6695166a4ec91179d1e48cf31c3d821cac6d955/bin/pkg_sql_tests benchdiff/d669516/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/d669516/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_write_only$ --old=d669516 --new=90c89be ./pkg/sql/testsArtifactsdownload: mkdir -p new
gcloud storage cp gs://cockroach-microbench-ci/artifacts/90c89be2b2b315abae3bf254c0aeb79a9e67b0f6/19086808714-1/\* new/
mkdir -p old
gcloud storage cp gs://cockroach-microbench-ci/artifacts/d6695166a4ec91179d1e48cf31c3d821cac6d955/19086808714-1/\* old/built with commit: 90c89be2b2b315abae3bf254c0aeb79a9e67b0f6 |
90c89be to
e21754f
Compare
Potential Bug(s) DetectedThe three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation. Next Steps: Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary. After you review the findings, please tag the issue as follows:
|
e21754f to
6be68d3
Compare
A new RecordedStatementStatsBuilder was added to the sql package to make building a RecordedStmtStats struct easier. New interfaces were also added to sqlstats to make make building RecordedStmtStats easier. These new interfaces and builders should help to decouple Recording statement stats from the conn executor, allowing them to be recorded in other places as well Epic: CRDB-55081 Release note: None
⚪ Sysbench [SQL, 3node, oltp_read_write]
Reproducebenchdiff binaries: mkdir -p benchdiff/6be68d3/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/6be68d3ebb8d8a2e17b6fe8709722dbbd4c1580c/bin/pkg_sql_tests benchdiff/6be68d3/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/6be68d3/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/d669516/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/d6695166a4ec91179d1e48cf31c3d821cac6d955/bin/pkg_sql_tests benchdiff/d669516/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/d669516/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: benchdiff --run=^BenchmarkSysbench/SQL/3node/oltp_read_write$ --old=d669516 --new=6be68d3 ./pkg/sql/tests⚪ Sysbench [KV, 3node, oltp_read_only]
Reproducebenchdiff binaries: mkdir -p benchdiff/6be68d3/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/6be68d3ebb8d8a2e17b6fe8709722dbbd4c1580c/bin/pkg_sql_tests benchdiff/6be68d3/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/6be68d3/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/d669516/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/d6695166a4ec91179d1e48cf31c3d821cac6d955/bin/pkg_sql_tests benchdiff/d669516/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/d669516/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_read_only$ --old=d669516 --new=6be68d3 ./pkg/sql/tests⚪ Sysbench [KV, 3node, oltp_write_only]
Reproducebenchdiff binaries: mkdir -p benchdiff/6be68d3/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/6be68d3ebb8d8a2e17b6fe8709722dbbd4c1580c/bin/pkg_sql_tests benchdiff/6be68d3/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/6be68d3/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/d669516/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/d6695166a4ec91179d1e48cf31c3d821cac6d955/bin/pkg_sql_tests benchdiff/d669516/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/d669516/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_write_only$ --old=d669516 --new=6be68d3 ./pkg/sql/testsArtifactsdownload: mkdir -p new
gcloud storage cp gs://cockroach-microbench-ci/artifacts/6be68d3ebb8d8a2e17b6fe8709722dbbd4c1580c/19111167798-1/\* new/
mkdir -p old
gcloud storage cp gs://cockroach-microbench-ci/artifacts/d6695166a4ec91179d1e48cf31c3d821cac6d955/19111167798-1/\* old/built with commit: 6be68d3ebb8d8a2e17b6fe8709722dbbd4c1580c |
6be68d3 to
27aa535
Compare
⚪ Sysbench [SQL, 3node, oltp_read_write]
Reproducebenchdiff binaries: mkdir -p benchdiff/27aa535/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/27aa535f2caf971d0cbd53ff76f04af4b8a69b87/bin/pkg_sql_tests benchdiff/27aa535/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/27aa535/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/d669516/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/d6695166a4ec91179d1e48cf31c3d821cac6d955/bin/pkg_sql_tests benchdiff/d669516/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/d669516/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: benchdiff --run=^BenchmarkSysbench/SQL/3node/oltp_read_write$ --old=d669516 --new=27aa535 ./pkg/sql/tests⚪ Sysbench [KV, 3node, oltp_read_only]
Reproducebenchdiff binaries: mkdir -p benchdiff/27aa535/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/27aa535f2caf971d0cbd53ff76f04af4b8a69b87/bin/pkg_sql_tests benchdiff/27aa535/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/27aa535/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/d669516/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/d6695166a4ec91179d1e48cf31c3d821cac6d955/bin/pkg_sql_tests benchdiff/d669516/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/d669516/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_read_only$ --old=d669516 --new=27aa535 ./pkg/sql/tests⚪ Sysbench [KV, 3node, oltp_write_only]
Reproducebenchdiff binaries: mkdir -p benchdiff/27aa535/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/27aa535f2caf971d0cbd53ff76f04af4b8a69b87/bin/pkg_sql_tests benchdiff/27aa535/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/27aa535/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/d669516/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/d6695166a4ec91179d1e48cf31c3d821cac6d955/bin/pkg_sql_tests benchdiff/d669516/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/d669516/bin/1058449141/cockroachdb_cockroach_pkg_sql_testsbenchdiff command: benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_write_only$ --old=d669516 --new=27aa535 ./pkg/sql/testsArtifactsdownload: mkdir -p new
gcloud storage cp gs://cockroach-microbench-ci/artifacts/27aa535f2caf971d0cbd53ff76f04af4b8a69b87/19115259300-1/\* new/
mkdir -p old
gcloud storage cp gs://cockroach-microbench-ci/artifacts/d6695166a4ec91179d1e48cf31c3d821cac6d955/19115259300-1/\* old/built with commit: 27aa535f2caf971d0cbd53ff76f04af4b8a69b87 |
Updates the UDF routine generators to pass down the necessary information in order to generate and record RecordedStmtStats structs Epic: CRDB-55081 Release note: None
Adds RoutineStatementLatencyRecorder struct that implements sqlstats.StatementLatencyRecorder. Now statement latencies are recorded Epic: CRDB-55081 Release note: None
27aa535 to
0a04ffd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mgartner reviewed 2 of 8 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball and @michae2)
-- commits line 8 at r1:
nit: break commit lines at 80 characters https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/73072807/Git+Commit+Messages
pkg/sql/sqlstats/ssprovider.go line 179 at r1 (raw file):
} func NewRecordedStatementStatsBuilder[L StatementLatencyRecorder, Q QueryStats, P PlanInfo](
I like the idea of cleaning up and narrowing the API. This use of interfaces and generics might be over-complicating things, though. Could this all be replaced by a struct(s) and a constructor(s) defined in the sqlstats package and a function(s) in the sql package to call those constructors with the data it has gathered from it's sources? What are the pros/cons of a simpler design?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @kyle-a-wong, and @michae2)
-- commits line 18 at r2:
nit: end sentence with a period
pkg/sql/routine.go line 386 at r2 (raw file):
stmt.StmtType, stmt.AppName, PlanInfo{ planFlags: g.p.curPlan.flags, // TODO: Is this right?
I don't think this is right. g.p is the top-level planner. It is copied inside runPlanInsidePlan and the copy is given a new curPlan representing the routine. We'll need to access that plan's flags, which should be plan.(*planComponents).flags.
pkg/sql/sem/tree/routine.go line 40 at r2 (raw file):
AppName string FingerprintId uint64 }
I don't understand the need to pass all this information into the routine execution code, just to construct a builder there. It looks like we only need to attach plan flags and record the statement within execution. Can we instead pass a pointer to a struct that has all this information already, and call a method to attach the plan flags and record the statement? Another option would be passing a closure recordStats func(flags planFlags) that the routine execution code can call with the generated plan flags.
commit 1/2
sql,*: record UDF stats
Updates the UDF routine generators to pass down
the necessary information in order to generate
and record RecordedStmtStats structs
Epic: CRDB-55081
Release note: None
commit 2/2
sql: Add latency recordings to udf sub statement calls
Adds RoutineStatementLatencyRecorder struct that implements
sqlstats.StatementLatencyRecorder. Now statement latencies
are recorded
Epic: CRDB-55081
Release note: None
note this is a stacked PR, so only the last 2 commits need to be reviewed