infoschema, session: support for events_statements_summary_by_digest #12017

djshow832 · 2019-09-04T03:31:39Z

What problem does this PR solve?

Support system table events_statements_summary_by_digest, which is used to summary statements by digest.

What is changed and how it works?

Add methods to perfSchemaTable, and add a new table events_statements_summary_by_digest.
Add a system variable tidb_enable_stmt_summary and add configurations max-stmt-count & max-sql-length.
Summary the statement execution information and put it into StmtSummary after a SQL runs.
Get information from StmtSummary when the user queries events_statements_summary_by_digest.

Check List

Tests

Unit test

Code changes

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

Side effects

Possible performance regression
Increased code complexity

Related changes

Need to cherry-pick to the release branch

codecov · 2019-09-04T03:40:28Z

Codecov Report

Merging #12017 into master will decrease coverage by 0.2214%.
The diff coverage is 94.2196%.

@@               Coverage Diff                @@
##             master     #12017        +/-   ##
================================================
- Coverage   81.4122%   81.1907%   -0.2215%     
================================================
  Files           454        454                
  Lines         99291      97973      -1318     
================================================
- Hits          80835      79545      -1290     
- Misses        12664      12707        +43     
+ Partials       5792       5721        -71

djshow832 · 2019-09-09T14:07:26Z

/bench

sre-bot · 2019-09-09T15:04:44Z

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: 1ef81a8620997103dd81fa2f74fd09a3a0683c1b
+++ tidb: e1e3cf3127b3e1eeb6d25aa507d0b541b1bbade1
tikv: 5ad30cba22f80853279c990b4030fb247f32b3ad
pd: c7c572a7dc9710aca57cb0bddf4d9bca6c4a111b
================================================================================
test-1: < oltp_insert >
    * QPS : 21432.92 ± 0.3035% (std=48.71) delta: 0.12%
    * AvgMs : 11.94 ± 0.2933% (std=0.03) delta: -0.14%
    * PercentileMs99 : 42.61 ± 0.0000% (std=0.00) delta: 0.00%
            
test-2: < oltp_update_non_index >
    * QPS : 29627.64 ± 0.3367% (std=62.74) delta: 0.05%
    * AvgMs : 8.64 ± 0.3241% (std=0.02) delta: -0.05%
    * PercentileMs99 : 30.59 ± 1.0788% (std=0.27) delta: -0.71%
            
test-3: < oltp_read_write >
    * QPS : 36883.62 ± 0.2365% (std=61.13) delta: -0.24%
    * AvgMs : 139.37 ± 0.2188% (std=0.21) delta: 0.24%
    * PercentileMs99 : 257.95 ± 0.0000% (std=0.00) delta: 0.00%
            
test-4: < oltp_point_select >
    * QPS : 74881.82 ± 3.0726% (std=1336.49) delta: -0.92%
    * AvgMs : 3.42 ± 3.2768% (std=0.06) delta: 0.97%
    * PercentileMs99 : 7.43 ± 0.0000% (std=0.00) delta: -0.70%
            
test-5: < oltp_update_index >
    * QPS : 17080.78 ± 0.2005% (std=22.25) delta: -0.02%
    * AvgMs : 14.98 ± 0.2169% (std=0.02) delta: -0.01%
    * PercentileMs99 : 47.99 ± 1.0877% (std=0.43) delta: 0.00%

https://perf.pingcap.com

sre-bot · 2019-09-09T17:51:54Z

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: bb5bfa4bd7f9b3bb67e06d3de14a82c61c2eb9d8
+++ tidb: e1e3cf3127b3e1eeb6d25aa507d0b541b1bbade1
tikv: 5ad30cba22f80853279c990b4030fb247f32b3ad
pd: c7c572a7dc9710aca57cb0bddf4d9bca6c4a111b
================================================================================
tidb_max_cpu: 19.40, delta: -0.50%
tikv_max_cpu: 13.15, delta: 3.45%
tidb_max_memory: 1784.65 MiB, delta: -1.13%
tikv_max_memory: 59841.52 MiB, delta: -0.02%

Measured tpmC (NewOrders): 21981.93, delta: 0.49%

https://perf.pingcap.com

jackysp · 2019-09-10T03:07:09Z

infoschema/perfschema/tables.go

@@ -14,10 +14,19 @@
 package perfschema

 import (
+	"fmt"


Plz add a blank line below this.

jackysp · 2019-09-10T03:10:09Z

infoschema/perfschema/tables.go

+)
+
+const (
+	tableNameEventsStatementsSummaryByDigest = "events_statements_summary_by_digest"


I am not sure if we should resurrect perf_schema, maybe we can put it to info_schema?

Some table names are the same, like tables.go.
And performance schema is independent with information schema in MySQL.
So I prefer the directory to be like this:

--virtual_table
----infoschema
----perfschema
----sys

But now, I prefer not to change the directory structure in this PR. Maybe fixing it in another PR is better, if needed.

jackysp · 2019-09-10T03:10:53Z

util/stmtsummary/statement_summary.go

@@ -0,0 +1,198 @@
+// Copyright 2016 PingCAP, Inc.


It is 2019 now.

jackysp · 2019-09-10T03:11:44Z

util/stmtsummary/statement_summary_test.go

@@ -0,0 +1,303 @@
+package stmtsummary


Add license and format imports.

config/config.go

config/config.toml.example

executor/adapter.go

bb7133 · 2019-09-10T03:31:25Z

infoschema/perfschema/tables.go

+
+// AddRecord implements table.Table AddRecord interface.
+func (vt *perfSchemaTable) AddRecord(ctx sessionctx.Context, r []types.Datum, opts ...table.AddRecordOption) (recordID int64, err error) {
+	fmt.Println("add record")


Please remove this manual-debugging code

bb7133 · 2019-09-10T03:31:35Z

infoschema/perfschema/tables.go

+
+// UpdateRecord implements table.Table UpdateRecord interface.
+func (vt *perfSchemaTable) UpdateRecord(ctx sessionctx.Context, h int64, oldData, newData []types.Datum, touched []bool) error {
+	fmt.Println("update record")


infoschema/perfschema/tables_test.go

sessionctx/variable/session.go

sessionctx/variable/tidb_vars.go

bb7133 · 2019-09-10T03:52:13Z

util/stmtsummary/statement_summary.go

+func NewStmtSummaryByDigest() *stmtSummaryByDigest {
+	maxStmtCount := config.GetGlobalConfig().StmtSummary.MaxStmtCount
+	return &stmtSummaryByDigest{
+		summaryMap: kvcache.NewSimpleLRUCache(maxStmtCount, 0, 0),


Is it possible that we still need memory quota, say, if the maxStmtCount is set to be very very large?

It complicates the configuration of statement summary, which makes users more confusing. If the user sets max-stmt-count to a very large value, he can also set the quota to a very large value.

util/stmtsummary/statement_summary.go

coocood · 2019-09-10T13:47:18Z

infoschema/perfschema/tables.go

+	return table.VirtualTable
+}
+
+func dataForEventsStatementsSummaryByDigest(ctx sessionctx.Context) [][]types.Datum {


We can inline this method.

coocood · 2019-09-10T13:52:59Z

infoschema/perfschema/tables.go

+}
+
+// RowWithCols implements table.Table RowWithCols interface.
+func (vt *perfSchemaTable) RowWithCols(ctx sessionctx.Context, h int64, cols []*table.Column) ([]types.Datum, error) {


Those unsupported methods are already implemented by dummy VirtualTable which is embedded in perfSchemaTable, we can remove those methods for perfSchemaTable.

coocood · 2019-09-11T03:13:43Z

util/stmtsummary/statement_summary.go

+
+// A LRU cache that stores statement summary.
+type stmtSummaryByDigest struct {
+	// It's rare to read concurrently, so RWMutex is not needed.


The comment contradicts the implementation.

I mean, we need Mutex instead of RWMutex.

Then why use RWMutex?

I'm not using RWMutex.

coocood · 2019-09-11T03:26:05Z

util/stmtsummary/statement_summary.go

+	ss.Unlock()
+
+	// Lock a single entry, not the whole cache.
+	if summary != nil {


Seems summary can not be nil.

If an entry is not found in the cache, we create newSummary, so summary stays nil.

Then why not define summary and addStmtExecInfoToSummary in the ok block?

Or we can create a new summary without adding the stats, call addStmtExecInfoToSummary later.

Separate addStmtExecInfoToSummary and the ok block because I want to move addStmtExecInfoToSummary out of the big cache lock, and add a smaller lock to it.
Why do I need to create a new summary?

So we can remove the if summary != nil check

The code is simplified a little.

coocood · 2019-09-11T03:26:55Z

util/stmtsummary/statement_summary.go

+	if sei.TotalLatency < ss.minLatency {
+		ss.minLatency = sei.TotalLatency
+	}
+	// TODO: uint64 overflow


We don't need to worry about it.

coocood · 2019-09-11T05:02:50Z

util/stmtsummary/statement_summary.go

+}
+
+// Add a new StmtExecInfo to stmtSummary
+func addStmtExecInfoToSummary(sei *StmtExecInfo, ss *stmtSummary) {


How about that we make this function a method of stmtSummary ?

change the function name to NewStmtSummary(sei *StmtExecInfo)

ss := new(stmtSummary); ss.convertFromExecInfo(sei)
which one?

func (ss *stmtSummary) add(sei *StmtExecInfo)

…y is set to false

bb7133

LGTM

bb7133 · 2019-09-12T11:48:22Z

/run-all-tests

sre-bot · 2019-09-12T11:48:44Z

/run-all-tests

sre-bot · 2019-09-12T11:53:56Z

cherry pick to release-3.0 failed

…ingcap#12017) cherry-pick 12217 to release-3.0

…12017 (#12306)

…ingcap#12017) cherry-pick 12017 to release-3.1

…_digest pingcap#12017 (pingcap#12306)" This reverts commit a3e1d01.

…12017 (#12328)

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

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.

sre-bot added the contribution This PR is from a community contributor. label Sep 4, 2019

djshow832 force-pushed the sql_monitor_by_digest branch from c012f4f to 3e63c5b Compare September 4, 2019 03:44

djshow832 added the status/WIP label Sep 4, 2019

djshow832 force-pushed the sql_monitor_by_digest branch from ce890f4 to e1e3cf3 Compare September 9, 2019 05:52

djshow832 force-pushed the sql_monitor_by_digest branch from e1e3cf3 to d15dbd2 Compare September 10, 2019 02:58

jackysp reviewed Sep 10, 2019

View reviewed changes

djshow832 added the needs-cherry-pick-3.0 label Sep 10, 2019

jackysp reviewed Sep 10, 2019

View reviewed changes

djshow832 removed the status/WIP label Sep 10, 2019

jackysp reviewed Sep 10, 2019

View reviewed changes

lysu added component/server type/usability type/new-feature and removed contribution This PR is from a community contributor. labels Sep 10, 2019

bb7133 reviewed Sep 10, 2019

View reviewed changes

lysu reviewed Sep 10, 2019

View reviewed changes

util/stmtsummary/statement_summary.go Outdated Show resolved Hide resolved

djshow832 force-pushed the sql_monitor_by_digest branch 2 times, most recently from 5d66398 to 2c6dcc7 Compare September 10, 2019 06:25

coocood reviewed Sep 10, 2019

View reviewed changes

coocood reviewed Sep 11, 2019

View reviewed changes

djshow832 force-pushed the sql_monitor_by_digest branch from 8d1c9f0 to f29aad4 Compare September 11, 2019 04:26

coocood reviewed Sep 11, 2019

View reviewed changes

djshow832 added 10 commits September 12, 2019 19:06

format imports in tables.go

2482938

remove debugging print in tables.go

6a3d209

add a separate lock for each summary entry

02cedb1

remove unsupported methods from perfSchemaTable

681e099

use Mutex in stmtSummaryByDigest instead of RWMutex

9dfb973

rename class names related to statement summary

8b2b298

clear events_statement_summary_by_digest when tidb_enable_stmt_summar…

f60ad41

…y is set to false

remove sysvar_observers to simplify code

2fde3ad

modify TestStmtSummaryTable to avoid copying lock value

06beeac

rephrase comments for statement summary

5e59a41

djshow832 force-pushed the sql_monitor_by_digest branch from d8ce20f to 5e59a41 Compare September 12, 2019 11:07

bb7133 reviewed Sep 12, 2019

View reviewed changes

bb7133 added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 12, 2019

bb7133 approved these changes Sep 12, 2019

View reviewed changes

bb7133 added the status/can-merge Indicates a PR has been approved by a committer. label Sep 12, 2019

sre-bot merged commit 0f55274 into pingcap:master Sep 12, 2019

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

infoschema, session: support for events_statements_summary_by_digest (p…

e7d11d3

…ingcap#12017) cherry-pick 12217 to release-3.0

djshow832 mentioned this pull request Sep 23, 2019

infoschema, session: support for events_statements_summary_by_digest #12017 #12306

Merged

sre-bot pushed a commit that referenced this pull request Sep 23, 2019

infoschema, session: support for events_statements_summary_by_digest #…

a3e1d01

…12017 (#12306)

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

infoschema, session: support for events_statements_summary_by_digest (p…

c35d919

…ingcap#12017) cherry-pick 12017 to release-3.1

djshow832 mentioned this pull request Sep 24, 2019

infoschema, session: support for events_statements_summary_by_digest (#12017) #12328

Merged

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

Revert "infoschema, session: support for events_statements_summary_by…

6c4d124

…_digest pingcap#12017 (pingcap#12306)" This reverts commit a3e1d01.

sre-bot pushed a commit that referenced this pull request Sep 29, 2019

infoschema, session: support for events_statements_summary_by_digest #…

1667c9e

…12017 (#12328)

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

infoschema, session: support for events_statements_summary_by_digest #12017

infoschema, session: support for events_statements_summary_by_digest #12017

Conversation

djshow832 commented Sep 4, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

codecov bot commented Sep 4, 2019 • edited Loading

Codecov Report

djshow832 commented Sep 9, 2019

sre-bot commented Sep 9, 2019

sre-bot commented Sep 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djshow832 Sep 10, 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

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

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

coocood Sep 11, 2019 • edited Loading

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

bb7133 commented Sep 12, 2019

sre-bot commented Sep 12, 2019

sre-bot commented Sep 12, 2019

sre-bot commented Apr 7, 2020

djshow832 commented Sep 4, 2019 •

edited

Loading

codecov bot commented Sep 4, 2019 •

edited

Loading

djshow832 Sep 10, 2019 •

edited

Loading

coocood Sep 11, 2019 •

edited

Loading