-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
infoschema: add plan field to the statement summary tables #14182
Conversation
/bench |
Benchmark Report
@@ Benchmark Diff @@
================================================================================
--- tidb: 8cbacf0d7c4612e6b5fa89836ca170af11eb81b2
+++ tidb: 012a939e38062d51f79191ce401fee29e8c5e5f9
tikv: 6ccfe673d60a19626ba19331500d661390ea305c
pd: 4d65bbefbc6db6e92fee33caa97415274512757a
================================================================================
oltp_update_non_index:
* QPS: 4720.55 ± 0.15% (std=4.92) delta: 0.16% (p=0.272)
* Latency p50: 27.11 ± 0.00% (std=0.00) delta: -0.17%
* Latency p99: 41.10 ± 0.00% (std=0.00) delta: -2.41%
oltp_insert:
* QPS: 4773.11 ± 0.29% (std=10.25) delta: 0.11% (p=0.503)
* Latency p50: 26.81 ± 0.27% (std=0.05) delta: -0.10%
* Latency p99: 45.85 ± 7.07% (std=2.24) delta: -0.81%
oltp_read_write:
* QPS: 15155.76 ± 0.07% (std=7.68) delta: 0.13% (p=0.755)
* Latency p50: 169.23 ± 0.08% (std=0.10) delta: -0.13%
* Latency p99: 307.67 ± 5.95% (std=11.48) delta: -4.42%
oltp_update_index:
* QPS: 4250.36 ± 0.10% (std=2.85) delta: 0.46% (p=0.275)
* Latency p50: 30.11 ± 0.09% (std=0.02) delta: -0.47%
* Latency p99: 53.87 ± 3.62% (std=1.53) delta: 0.04%
oltp_point_select:
* QPS: 38721.92 ± 0.32% (std=85.51) delta: 0.65% (p=0.035)
* Latency p50: 3.30 ± 0.30% (std=0.01) delta: -0.68%
* Latency p99: 10.00 ± 0.90% (std=0.09) delta: -0.45%
|
/run-all-tests |
8a0324b
to
3e342f8
Compare
/run-all-tests |
key.hash = append(key.hash, hack.Slice(key.digest)...) | ||
key.hash = append(key.hash, hack.Slice(key.schemaName)...) | ||
key.hash = append(key.hash, hack.Slice(key.prevDigest)...) | ||
key.hash = append(key.hash, hack.Slice(key.planDigest)...) |
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.
The same plan but different row count may cause the hash different, is it expected?
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.
No, row count is not in normalized plan, so plan digest are the same.
Refer to https://github.com/pingcap/tidb/blob/master/util/plancodec/codec.go#L274
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.
Add some tests to cover this case (different plan but the same plan digest).
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.
Addressed. Refer to 00cbf9e
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.
Rest LGTM
3e342f8
to
2e0836c
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.
LGTM
(Prefer to use a variable to store the failpoint name)
/run-unit-test |
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.
LGTM.
BTW, The plan no need to store the real plan, store the normalized plan is ok. because store the plan need encode plan and will user snappy compress the plan, it will have some performance impact?
executor/adapter.go
Outdated
@@ -876,6 +876,9 @@ func (a *ExecStmt) SummaryStmt() { | |||
} | |||
sessVars.SetPrevStmtDigest(digest) | |||
|
|||
plan := plannercore.EncodePlan(a.Plan) |
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.
No performance impact? can we use the normalized plan directly?
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.
But it look ok from the result of benchmark Report...
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.
I changed it to recording the plan only at the first time.
I thought about it, but normalized plan is hard to read. E.g.
I don't know what I'm thinking about printing the plan only at the first time, but I also want to make fields |
sampleSQL := sei.OriginalSQL | ||
if len(sampleSQL) > int(maxSQLLength) { | ||
// Make sure the memory of original `sampleSQL` will be released. | ||
sampleSQL = string([]byte(sampleSQL[:maxSQLLength])) |
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.
Do we need to attach the information of sample query total length, e.g: select * from tt (total: 2304bytes)
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.
Addressed.
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.
LGTM
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.
LGTM
/merge |
/run-all-tests |
/run-cherry-picker |
cherry pick to release-3.0 in PR #14285 |
What problem does this PR solve?
Add plan and plan digest to the statement summary tables.
Same SQLs with different plans are summarized in different records in the tables.
Record the SQL and plan that appear at the first time in each summary, not the last time, to increase performance.
Since same SQL may be split into several records,
max-stmt-count
should increase.What is changed and how it works?
Add
plan_digest
andplan
fields.schema_name
+digest
+prev_sql_digest
+plan_digest
are combined as a key of the summary map.Default value of
max-stmt-count
is increased to 200.Record the SQL and plan that appear at the first time in each summary, not the last time.
Check List
Tests
Code changes
Side effects
Related changes
Release note
plan
andplan_digest
to the statement summary tables.max-stmt-count
in [stmt-summary] to 200.