infoschema, util: add table `events_statements_summary_by_digest_history` #13813

djshow832 · 2019-11-29T04:23:45Z

What problem does this PR solve?

Add a history table for statement summary for storing summaries that expired from events_statements_summary_by_digest.

What is changed and how it works?

Add a virtual table events_statements_summary_by_digest_history. When a summary expires from events_statements_summary_by_digest, put it into events_statements_summary_by_digest_history.
There's a system variable tidb_stmt_summary_history_size to control the history size for each summary.

Check List

Tests

Unit test
Manual test

Code changes

Has exported function/method change
Has interface methods change

Side effects

Possible performance regression
Increased code complexity

Related changes

Need to cherry-pick to the release branch
Need to update the documentation

Release note

Add a history table events_statements_summary_by_digest_history to store history summaries of events_statements_summary_by_digest.

djshow832 · 2019-11-29T04:23:59Z

/bench

sre-bot · 2019-11-29T05:51:48Z

Benchmark Report

Run Sysbench Performance Test on VMs

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: 1c65ab936f270dd831b7923f7e1feb2b7ae6b4d1
+++ tidb: 5ed1ffbb6737f248dcb79dd64a3912194e9ab221
tikv: f6869aff3587ba0bcec47e099e57fdf11f63cbaf
pd: a87c9eb3181eb99cd6ae0128d07bad6d99b0c579
================================================================================
oltp_update_non_index:
    * QPS: 4717.87 ± 0.18% (std=5.80) delta: 0.08% (p=0.715)
    * Latency p50: 27.13 ± 0.00% (std=0.00) delta: -0.08%
    * Latency p99: 41.68 ± 4.11% (std=1.12) delta: -1.83%
            
oltp_insert:
    * QPS: 4779.43 ± 0.45% (std=14.57) delta: -0.03% (p=0.888)
    * Latency p50: 26.77 ± 0.47% (std=0.08) delta: 0.04%
    * Latency p99: 44.85 ± 8.36% (std=2.63) delta: 1.49%
            
oltp_read_write:
    * QPS: 15459.98 ± 0.58% (std=63.63) delta: 0.14% (p=0.679)
    * Latency p50: 165.91 ± 0.64% (std=0.72) delta: -0.17%
    * Latency p99: 300.92 ± 6.40% (std=13.55) delta: -1.40%
            
oltp_update_index:
    * QPS: 4267.52 ± 0.33% (std=9.61) delta: 0.32% (p=0.105)
    * Latency p50: 29.99 ± 0.32% (std=0.07) delta: -0.22%
    * Latency p99: 52.27 ± 2.39% (std=0.88) delta: -2.94%
            
oltp_point_select:
    * QPS: 40097.44 ± 0.25% (std=88.29) delta: 0.87% (p=0.004)
    * Latency p50: 3.19 ± 0.31% (std=0.01) delta: -0.85%
    * Latency p99: 9.82 ± 0.92% (std=0.09) delta: 0.00%

djshow832 · 2019-12-02T03:05:38Z

/run-all-tests

djshow832 · 2019-12-02T03:53:20Z

/run-unit-test

codecov · 2019-12-02T03:56:24Z

Codecov Report

Merging #13813 into master will decrease coverage by 0.0124%.
The diff coverage is 92.4187%.

@@               Coverage Diff                @@
##             master     #13813        +/-   ##
================================================
- Coverage   80.1862%   80.1738%   -0.0125%     
================================================
  Files           481        481                
  Lines        120790     120593       -197     
================================================
- Hits          96857      96684       -173     
+ Misses        16224      16187        -37     
- Partials       7709       7722        +13

infoschema/perfschema/const.go

crazycs520 · 2019-12-03T13:25:13Z

util/stmtsummary/statement_summary.go

+// historySize gets the history size for summaries.
+func (ssMap *stmtSummaryByDigestMap) historySize() int {
+	size := int(atomic.LoadInt32(&ssMap.sysVars.historySize))
+	if size <= 0 {


Redundant check?
Already do the check when setting the value. line#445

Agree, we should set history size in SetHistorySize at once.

It's normal that SetHistorySize is always called after bootstrap. But I do the check just in case that there would be any bug that prevents SetHistorySize be called after bootstrap, and it'll be a big problem.

Now I find out the problem.
If TiDB is just upgraded, global variables stored in TiKV doesn't include tidb_stmt_summary_history_size, so the loaded GlobalVariableCache doesn't include this variable, and then historySize won't be initialized.
I'll fix it.

crazycs520 · 2019-12-03T13:28:31Z

util/stmtsummary/statement_summary.go

+func (ssMap *stmtSummaryByDigestMap) historySize() int {
+	size := int(atomic.LoadInt32(&ssMap.sysVars.historySize))
+	if size <= 0 {
+		size = 1


Why the default value of historySize is 1? Should be 0?

or DefTiDBStmtSummaryHistorySize?

Default value is DefTiDBStmtSummaryHistorySize. Just in case that historySize is never set.

Addressed. Now I make sure that historySize is always initialized, so this judgement is removed.

infoschema/perfschema/tables_test.go

crazycs520 · 2019-12-04T03:52:47Z

util/stmtsummary/statement_summary.go

+			size = 0
+		}
+	}
+	if size > 0 {


historySize will always not set to 0？

after

set @@session.tidb_stmt_summary_history_size=0; set @@global.tidb_stmt_summary_history_size=0;

, the historySize still not 0?

Yes. Actually, history includes the current summary. So historySize=1 indicates that there's only the current summary, and historySize=0 makes no sense.
I designed it like this because I wish xxx_history would be enough to see all the summaries.
What do you think?

Addressed. Now historySize is allowed to be 0.

util/stmtsummary/statement_summary.go

crazycs520 · 2019-12-04T03:57:20Z

util/stmtsummary/statement_summary.go

 	return ssbd
 }

-func (ssbd *stmtSummaryByDigest) add(sei *StmtExecInfo) {
+func (ssbd *stmtSummaryByDigest) add(sei *StmtExecInfo, beginTime int64, historySize int) {
+	// Enclose this block in a function to ensure the lock will always be released.


return if historySize == 0?

Good idea. Addressed.

? how addressed?

bb7133 · 2019-12-04T16:07:51Z

util/stmtsummary/statement_summary.go

+		defer ssbd.Unlock()
+
+		var ssElement *stmtSummaryByDigestElement
+		ok := false


The logic is tricky here, please rename this variable and add some comments

bb7133 · 2019-12-04T16:25:39Z

util/stmtsummary/statement_summary.go

+
+		// `historySize` might be modified, so check expiration every time.
+		if historySize == 0 {
+			historySize = 1


This assignment looks weird. Why reassign historySize here?

If historySize is 0, displayed history summaries in events_statements_summary_by_digest_history should be empty, but summary cannot be cleared, because table events_statements_summary_by_digest still needs it.
Alternatively, I can code for ssbd.history.Len() > historySize && ssbd.history.Len() > 1 {...}. But it looks also weird.

sessionctx/variable/varsutil.go

util/stmtsummary/statement_summary.go

bb7133

LGTM

crazycs520

LGTM

djshow832 · 2019-12-09T06:04:40Z

/run-all-tests

…ory` (pingcap#13813)

…ory` (#13813) (#14166)

…ory` (pingcap#13813)

djshow832 added status/DNM type/usability component/util labels Nov 29, 2019

djshow832 force-pushed the stmt_summary_history branch from 5ed1ffb to 8e2d491 Compare November 29, 2019 10:07

djshow832 removed the status/DNM label Nov 29, 2019

djshow832 force-pushed the stmt_summary_history branch from 8e2d491 to 4a264a5 Compare December 2, 2019 02:50

djshow832 requested review from zimulala, AilinKid, bb7133 and crazycs520 December 2, 2019 04:48

crazycs520 reviewed Dec 3, 2019

View reviewed changes

infoschema/perfschema/const.go Show resolved Hide resolved

crazycs520 reviewed Dec 3, 2019

View reviewed changes

bb7133 reviewed Dec 3, 2019

View reviewed changes

infoschema/perfschema/tables_test.go Show resolved Hide resolved

crazycs520 reviewed Dec 4, 2019

View reviewed changes

util/stmtsummary/statement_summary.go Show resolved Hide resolved

crazycs520 reviewed Dec 4, 2019

View reviewed changes

djshow832 force-pushed the stmt_summary_history branch 3 times, most recently from eec2e5c to 09e49b3 Compare December 4, 2019 06:44

bb7133 reviewed Dec 4, 2019

View reviewed changes

djshow832 force-pushed the stmt_summary_history branch from 09e49b3 to 93471b8 Compare December 5, 2019 03:17

crazycs520 reviewed Dec 5, 2019

View reviewed changes

sessionctx/variable/varsutil.go Outdated Show resolved Hide resolved

crazycs520 reviewed Dec 5, 2019

View reviewed changes

util/stmtsummary/statement_summary.go Show resolved Hide resolved

crazycs520 reviewed Dec 5, 2019

View reviewed changes

util/stmtsummary/statement_summary.go Show resolved Hide resolved

djshow832 force-pushed the stmt_summary_history branch from 93471b8 to a5a6baf Compare December 6, 2019 03:49

bb7133 reviewed Dec 9, 2019

View reviewed changes

bb7133 added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 9, 2019

djshow832 force-pushed the stmt_summary_history branch from a5a6baf to 6e5fd44 Compare December 9, 2019 02:59

crazycs520 previously approved these changes Dec 9, 2019

View reviewed changes

djshow832 added 11 commits December 9, 2019 15:57

add history table

f263b53

add test cases

9d15944

fix check_dev

0f2fed7

init variables at startup

dafb6f9

add config test

709276e

fix test case

ef99c99

allow history size to be 0

4761100

fix UT

2f7bdfe

address comments

327a51e

add config valid

3fc7df2

refractor default config

0a164dd

djshow832 dismissed crazycs520’s stale review via 0a164dd December 9, 2019 07:57

djshow832 force-pushed the stmt_summary_history branch from 6e5fd44 to 0a164dd Compare December 9, 2019 07:57

crazycs520 approved these changes Dec 9, 2019

View reviewed changes

djshow832 merged commit d439a57 into pingcap:master Dec 9, 2019

djshow832 added a commit to djshow832/tidb that referenced this pull request Dec 20, 2019

infoschema, util: add table `events_statements_summary_by_digest_hist…

9980e91

…ory` (pingcap#13813)

zimulala pushed a commit that referenced this pull request Dec 20, 2019

infoschema, util: add table `events_statements_summary_by_digest_hist…

45c52c4

…ory` (#13813) (#14166)

XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019

infoschema, util: add table `events_statements_summary_by_digest_hist…

cd4ac06

…ory` (pingcap#13813)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

infoschema, util: add table `events_statements_summary_by_digest_history` #13813

infoschema, util: add table `events_statements_summary_by_digest_history` #13813

djshow832 commented Nov 29, 2019

djshow832 commented Nov 29, 2019

sre-bot commented Nov 29, 2019

djshow832 commented Dec 2, 2019

djshow832 commented Dec 2, 2019

codecov bot commented Dec 2, 2019 •

edited

Loading

crazycs520 Dec 3, 2019

bb7133 Dec 3, 2019

djshow832 Dec 4, 2019

djshow832 Dec 4, 2019

djshow832 Dec 4, 2019

crazycs520 Dec 3, 2019

crazycs520 Dec 3, 2019

djshow832 Dec 4, 2019

djshow832 Dec 4, 2019 •

edited

Loading

crazycs520 Dec 4, 2019

crazycs520 Dec 4, 2019

djshow832 Dec 4, 2019

djshow832 Dec 4, 2019

crazycs520 Dec 4, 2019

djshow832 Dec 4, 2019

crazycs520 Dec 5, 2019

bb7133 Dec 4, 2019

djshow832 Dec 5, 2019

bb7133 Dec 4, 2019

djshow832 Dec 5, 2019

djshow832 Dec 5, 2019

bb7133 left a comment

crazycs520 left a comment

djshow832 commented Dec 9, 2019

infoschema, util: add table events_statements_summary_by_digest_history #13813

infoschema, util: add table events_statements_summary_by_digest_history #13813

Conversation

djshow832 commented Nov 29, 2019

What problem does this PR solve?

What is changed and how it works?

Check List

djshow832 commented Nov 29, 2019

sre-bot commented Nov 29, 2019

Benchmark Report

djshow832 commented Dec 2, 2019

djshow832 commented Dec 2, 2019

codecov bot commented Dec 2, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djshow832 Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bb7133 left a comment

Choose a reason for hiding this comment

crazycs520 left a comment

Choose a reason for hiding this comment

djshow832 commented Dec 9, 2019

infoschema, util: add table `events_statements_summary_by_digest_history` #13813

infoschema, util: add table `events_statements_summary_by_digest_history` #13813

codecov bot commented Dec 2, 2019 •

edited

Loading

djshow832 Dec 4, 2019 •

edited

Loading