session, executor: support setting tidb_enable_stmt_summary in session scope #12217

djshow832 · 2019-09-17T05:28:51Z

What problem does this PR solve?

Support setting tidb_enable_stmt_summary in session scope. It was only supported in global scope. Actually, session scope here equals server scope, and global scope here equals cluster scope.

What is changed and how it works?

When statement summary is enabled in global scope, it'll work immediately on the current server and work in 2 seconds on the other servers in the cluster.
When statement summary is enabled in session scope, it'll work immediately on the current server, and subsequent settings in the global scope won't work.
If tidb_enable_stmt_summary is set to '' in session scope, then tidb_enable_stmt_summary in global scope works in the current server.

Check List

Tests

Unit test

Code changes

Has exported function/method change
Has exported variable/fields change

Side effects

Possible performance regression
Increased code complexity

Related changes

Need to cherry-pick to the release branch

Release Note

Support system table performance_schema.events_statements_summary_by_digest. The table is used to summarize statements by digest of statements.

djshow832 · 2019-09-17T05:29:34Z

/bench

sre-bot · 2019-09-17T06:14:11Z

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: d438e103bea6ab98c2406903980e2077cce7652c
+++ tidb: c09f1bed50dc328e99d29b476424fa7b3e573797
tikv: 8519bf55a00197f6304c8a99580d437248d47601
pd: 06e2034f4f8c58f04ce293180c1eaf515424a58e
================================================================================
test-1: < oltp_insert >
    * QPS : 21336.35 ± 0.0908% (std=13.70) delta: 0.72%
    * AvgMs : 11.99 ± 0.0556% (std=0.00) delta: -0.73%
    * PercentileMs99 : 43.23 ± 2.1650% (std=0.58) delta: -0.72%
            
test-2: < oltp_update_non_index >
    * QPS : 29117.58 ± 0.2031% (std=36.56) delta: -0.23%
    * AvgMs : 8.78 ± 0.0854% (std=0.00) delta: 0.14%
    * PercentileMs99 : 31.37 ± 0.0000% (std=0.00) delta: 1.82%
            
test-3: < oltp_read_write >
    * QPS : 36857.90 ± 0.8002% (std=180.47) delta: 0.19%
    * AvgMs : 139.45 ± 0.7974% (std=0.69) delta: -0.19%
    * PercentileMs99 : 260.76 ± 1.0791% (std=2.30) delta: -0.71%
            
test-4: < oltp_point_select >
    * QPS : 75552.29 ± 1.4060% (std=591.27) delta: 0.28%
    * AvgMs : 3.39 ± 1.2397% (std=0.02) delta: 0.06%
    * PercentileMs99 : 7.56 ± 0.0000% (std=0.00) delta: 0.00%
            
test-5: < oltp_update_index >
    * QPS : 16945.28 ± 0.1970% (std=23.14) delta: 0.42%
    * AvgMs : 15.11 ± 0.1655% (std=0.02) delta: -0.43%
    * PercentileMs99 : 48.34 ± 0.0000% (std=0.00) delta: -0.71%

https://perf.pingcap.com

bb7133 · 2019-09-17T06:46:42Z

domain/global_vars_cache_test.go

@@ -14,7 +14,7 @@
 package domain

 import (
-	"sync/atomic"
+	"github.com/pingcap/tidb/util/stmtsummary"


Please re-arrange the order the imported package

codecov · 2019-09-17T06:54:37Z

Codecov Report

Merging #12217 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #12217   +/-   ##
===========================================
  Coverage   81.0924%   81.0924%           
===========================================
  Files           454        454           
  Lines         98606      98606           
===========================================
  Hits          79962      79962           
  Misses        12876      12876           
  Partials       5768       5768

djshow832 · 2019-09-17T10:24:22Z

/bench

…alse

coocood · 2019-09-17T10:52:42Z

util/stmtsummary/statement_summary.go

+// isEnabled converts a string value to bool.
+func (ssMap *stmtSummaryByDigestMap) isEnabled(enable string) bool {
+	switch {
+	case strings.EqualFold(enable, "ON") || enable == "1":


We can covert "ON" to "1" when setting the value.

Seems already converted in ValidateSetSystemVar.
We can simply return enable == "1".

ValidateSetSystemVar is executed in variable.SetSessionSystemVar, but variable.SetSessionSystemVar doesn't return converted value in SetExecutor.setSysVariable, so stmtsummary.StmtSummaryByDigestMap gets a raw value and a conversion is needed.

I added normalizeEnableValue to convert.

coocood · 2019-09-17T10:54:06Z

util/stmtsummary/statement_summary.go

+		sessionEnabled string
+		// setInSession indicates whether statement summary has been set in any session.
+		globalEnabled  string
+		defaultEnabled bool


I think we can remove defaultEnabled.
If the value is not 1, then enabled is false.

Default value should be defined once in the whole project so that it's convenient to modify.
Since DefTiDBEnableStmtSummary is already defined and is used in variable.defaultSysVars, we should reuse DefTiDBEnableStmtSummary as the default value in stmtsummary.

The global default is used once it is loaded, the DefTiDBEnableStmtSummary is not needed.

sre-bot · 2019-09-17T11:37:18Z

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: c5cad51459bf6967685fbea3c56553b7efb8cdbe
+++ tidb: 007b3d8b70e44a8092134e90fe97c0e61604f1f6
tikv: 2364333636896356a9b3c48d33807951d73d02ed
pd: cfa5706aec6466da3d2a2c4d0c6e713779fde67e
================================================================================
test-1: < oltp_read_write >
    * QPS : 37340.83 ± 0.8921% (std=246.42) delta: 1.42%
    * AvgMs : 137.66 ± 0.8978% (std=0.91) delta: -1.40%
    * PercentileMs99 : 257.95 ± 0.0000% (std=0.00) delta: -1.08%
            
test-2: < oltp_point_select >
    * QPS : 84005.32 ± 1.5337% (std=1024.54) delta: 1.56%
    * AvgMs : 3.05 ± 1.4445% (std=0.04) delta: -1.61%
    * PercentileMs99 : 5.84 ± 1.1309% (std=0.05) delta: -1.85%
            
test-3: < oltp_insert >
    * QPS : 21413.20 ± 0.3090% (std=44.27) delta: 0.60%
    * AvgMs : 11.95 ± 0.3137% (std=0.02) delta: -0.58%
    * PercentileMs99 : 42.92 ± 1.0903% (std=0.38) delta: -1.08%
            
test-4: < oltp_update_index >
    * QPS : 16917.79 ± 0.6630% (std=85.11) delta: -0.05%
    * AvgMs : 15.13 ± 0.6478% (std=0.08) delta: 0.04%
    * PercentileMs99 : 48.86 ± 1.0683% (std=0.43) delta: -0.37%
            
test-5: < oltp_update_non_index >
    * QPS : 29082.91 ± 0.4025% (std=76.67) delta: -0.01%
    * AvgMs : 8.80 ± 0.4545% (std=0.02) delta: 0.02%
    * PercentileMs99 : 31.15 ± 1.0788% (std=0.27) delta: -0.71%

https://perf.pingcap.com

coocood · 2019-09-17T12:18:03Z

util/stmtsummary/statement_summary.go

-		atomic.StoreInt32(&variable.EnableStmtSummary, 1)
+// SetEnabled enables or disables statement summary in global(cluster) or session(server) scope.
+func (ssMap *stmtSummaryByDigestMap) SetEnabled(enable string, inSession bool) {
+	enable = ssMap.normalizeEnableValue(enable)


We don't need to do this, the enable string already normalized by ValidateSetSystemVar

ValidateSetSystemVar is executed in variable.SetSessionSystemVar, but variable.SetSessionSystemVar doesn't return converted value in SetExecutor.setSysVariable, so stmtsummary.StmtSummaryByDigestMap gets a raw value and a conversion is needed.

coocood · 2019-09-17T12:30:58Z

util/stmtsummary/statement_summary.go

+		if !ssMap.isSet(ssMap.enabledWrapper.sessionEnabled) {
+			needClear = !ssMap.isEnabled(enable)
+		}
+	}


Maybe this is simpler.

ssMap.ssMap.enabledWrapper.Lock() if inSession { ssMap.enabledWrapper.sessionEnabled = enable } else { ssMap.enabledWrapper.globalEnabled = enable } sessionEnabled := ssMap.enabledWrapper.sessionEnabled globalEnabled := ssMap.enabledWrapper.globalEnabled ssMap.ssMap.enabledWrapper.UnLock() if ssMap.IsSet(sessionEnable) { needClear = !ssMap.IsEnabled(sessionEnable) } else { needClear = !ssMap.IsEnabled(globalEnable) }

coocood · 2019-09-17T12:46:43Z

LGTM

bb7133

For all PRs that need cherry-pick, please add a short "Release Note" to the PR Description.

You can refer to the released notes from previous version:

https://github.com/pingcap/tidb/releases

bb7133 · 2019-09-18T02:04:02Z

util/stmtsummary/statement_summary.go

+
+// isEnabled converts a string value to bool.
+// 1 indicates true, 0 or '' indicates false.
+func (ssMap *stmtSummaryByDigestMap) isEnabled(enable string) bool {


Suggested change

func (ssMap *stmtSummaryByDigestMap) isEnabled(enable string) bool {

func (ssMap *stmtSummaryByDigestMap) isEnabled(value string) bool {

How about renaming to "value"? "enable" is a verb.

bb7133 · 2019-09-18T02:04:13Z

util/stmtsummary/statement_summary.go

+}
+
+// isSet judges whether the variable is set.
+func (ssMap *stmtSummaryByDigestMap) isSet(enable string) bool {


bb7133

LGTM

sre-bot · 2019-09-18T06:45:44Z

/run-all-tests

djshow832 · 2019-09-18T07:08:31Z

/run-all-tests

sre-bot · 2019-09-18T07:22:04Z

cherry pick to release-3.0 failed

sre-bot · 2019-09-18T07:24:54Z

cherry pick to release-3.1 failed

…n scope (pingcap#12217) cherry-pick 12217 to release-3.0

…n scope (#12217) (#12308)

…n session scope (pingcap#12217) (pingcap#12308)" This reverts commit f33c908.

…n scope (pingcap#12217) (pingcap#12308) cherry-pick 12308 to release-3.1

sre-bot · 2020-04-07T10:23:53Z

It seems that, not for sure, we failed to cherry-pick this commit to release-3.0. Please comment '/run-cherry-picker' to try to trigger the cherry-picker if we did fail to cherry-pick this commit before. @djshow832 PTAL.

add session scope tidb_enable_stmt_summary

c09f1be

djshow832 added sig/execution SIG execution type/new-feature component/session needs-cherry-pick-3.0 component/util labels Sep 17, 2019

modify TestCheckEnableStmtSummary to adjust stmtsummary

6c2151a

bb7133 reviewed Sep 17, 2019

View reviewed changes

format imports in global_vars_cache_test

fc0be6e

djshow832 force-pushed the session_enable_stmt_summary branch from 350a256 to fc0be6e Compare September 17, 2019 06:54

support unset tidb_enable_stmt_summary in session scope

2879ab6

clear table if session scope variable is unset and global is set to f…

4910651

…alse

djshow832 force-pushed the session_enable_stmt_summary branch from 007b3d8 to 4910651 Compare September 17, 2019 10:49

coocood reviewed Sep 17, 2019

View reviewed changes

normalize value of tidb_enable_stmt_summary to simplify reading

44a7d0d

remove DefTidbEnableStmtSummary and use false as default

113b86b

coocood reviewed Sep 17, 2019

View reviewed changes

simplify SetEnabled method

e869935

djshow832 force-pushed the session_enable_stmt_summary branch from ba19b41 to e869935 Compare September 17, 2019 12:44

bb7133 reviewed Sep 18, 2019

View reviewed changes

rename variable enable to value

fd8ec51

djshow832 force-pushed the session_enable_stmt_summary branch from 07160ff to fd8ec51 Compare September 18, 2019 02:38

djshow832 added the needs-cherry-pick-3.1 label Sep 18, 2019

Merge branch 'master' into session_enable_stmt_summary

15dd7c3

bb7133 added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. labels Sep 18, 2019

bb7133 approved these changes Sep 18, 2019

View reviewed changes

Merge branch 'master' into session_enable_stmt_summary

49c8e48

bb7133 merged commit 9064c49 into pingcap:master Sep 18, 2019

djshow832 added a commit to djshow832/tidb that referenced this pull request Sep 23, 2019

session, executor: support setting tidb_enable_stmt_summary in sessio…

a7a66c5

…n scope (pingcap#12217) cherry-pick 12217 to release-3.0

djshow832 added a commit to djshow832/tidb that referenced this pull request Sep 23, 2019

session, executor: support setting tidb_enable_stmt_summary in sessio…

408d776

…n scope (pingcap#12217) cherry-pick 12217 to release-3.0

djshow832 mentioned this pull request Sep 23, 2019

session, executor: support setting tidb_enable_stmt_summary in session scope (#12217) #12308

Merged

winkyao pushed a commit that referenced this pull request Sep 23, 2019

session, executor: support setting tidb_enable_stmt_summary in sessio…

f33c908

…n scope (#12217) (#12308)

bb7133 added a commit to bb7133/tidb that referenced this pull request Sep 26, 2019

Revert "session, executor: support setting tidb_enable_stmt_summary i…

967d71f

…n session scope (pingcap#12217) (pingcap#12308)" This reverts commit f33c908.

djshow832 added a commit to djshow832/tidb that referenced this pull request Sep 30, 2019

session, executor: support setting tidb_enable_stmt_summary in sessio…

dd3ad59

…n scope (pingcap#12217) (pingcap#12308) cherry-pick 12308 to release-3.1

This was referenced Oct 9, 2019

reference: add documentation for statement summary pingcap/docs-cn#1923

Closed

reference: add documentation for statement summary pingcap/docs-cn#1939

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

session, executor: support setting tidb_enable_stmt_summary in session scope #12217

session, executor: support setting tidb_enable_stmt_summary in session scope #12217

djshow832 commented Sep 17, 2019 •

edited

Loading

djshow832 commented Sep 17, 2019

sre-bot commented Sep 17, 2019

bb7133 Sep 17, 2019

djshow832 Sep 17, 2019

codecov bot commented Sep 17, 2019 •

edited

Loading

djshow832 commented Sep 17, 2019

coocood Sep 17, 2019

coocood Sep 17, 2019

djshow832 Sep 17, 2019

djshow832 Sep 17, 2019

coocood Sep 17, 2019

djshow832 Sep 17, 2019

coocood Sep 17, 2019

djshow832 Sep 17, 2019

sre-bot commented Sep 17, 2019

coocood Sep 17, 2019

djshow832 Sep 17, 2019

coocood Sep 17, 2019

djshow832 Sep 17, 2019

coocood commented Sep 17, 2019

bb7133 left a comment

bb7133 Sep 18, 2019

djshow832 Sep 18, 2019

bb7133 Sep 18, 2019

djshow832 Sep 18, 2019

bb7133 left a comment

sre-bot commented Sep 18, 2019

djshow832 commented Sep 18, 2019

sre-bot commented Sep 18, 2019

sre-bot commented Sep 18, 2019

sre-bot commented Apr 7, 2020

	func (ssMap *stmtSummaryByDigestMap) isEnabled(enable string) bool {
	func (ssMap *stmtSummaryByDigestMap) isEnabled(value string) bool {

session, executor: support setting tidb_enable_stmt_summary in session scope #12217

session, executor: support setting tidb_enable_stmt_summary in session scope #12217

Conversation

djshow832 commented Sep 17, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

djshow832 commented Sep 17, 2019

sre-bot commented Sep 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 17, 2019 • edited Loading

Codecov Report

djshow832 commented Sep 17, 2019

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

sre-bot commented Sep 17, 2019

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

coocood commented Sep 17, 2019

bb7133 left a 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

sre-bot commented Sep 18, 2019

djshow832 commented Sep 18, 2019

sre-bot commented Sep 18, 2019

sre-bot commented Sep 18, 2019

sre-bot commented Apr 7, 2020

djshow832 commented Sep 17, 2019 •

edited

Loading

codecov bot commented Sep 17, 2019 •

edited

Loading