domain,executor: store topN slow query in domain #7646

tiancaiamao · 2018-09-08T09:10:09Z

What problem does this PR solve?

Store topN slow query in domain, so later we can retrieve it later.

What is changed and how it works?

logSlowQuery in session will send a copy to domain, the domain maintains a heap to store the
topN recent slow queries.

Check List

Tests

Unit test

shenli · 2018-09-09T08:05:59Z

Why put it in the domain?

winkyao · 2018-09-10T03:58:28Z

If put it in domain, it may lead to cycle import?

tiancaiamao · 2018-09-10T08:42:34Z

There is no cycle import. executor imports domain, domain doesn't imports executor. @winkyao

So if we don't put it in domain, where should we put it? @shenli

coocood · 2018-09-10T11:32:09Z

domain/domain.go

@@ -471,6 +514,7 @@ func NewDomain(store kv.Storage, ddlLease time.Duration, statsLease time.Duratio
 		sysSessionPool:  pools.NewResourcePool(factory, capacity, capacity, resourceIdleTimeout),
 		statsLease:      statsLease,
 		infoHandle:      infoschema.NewHandle(store),
+		slowQuery:       newTopNSlowQuery(30, time.Hour*24*7),


This should be configurable.

I agree, but we can do it in another PR.

coocood · 2018-09-10T11:36:46Z

domain/topn_slow_query.go

+	}
+}
+
+func (q *topNSlowQuery) shiftUp(end int) {


coocood · 2018-09-10T11:42:01Z

This is not a performance hot spot, we can just use heap in the standard library.

tiancaiamao · 2018-09-10T12:57:02Z

PTAL @coocood @winkyao

coocood · 2018-09-10T13:14:34Z

domain/topn_slow_query.go

+
+// topNSlowQuery maintains a heap to store recent slow queries.
+// N = 30, recent = 7 days by default.
+type topNSlowQuery struct {


There are multiple query entries, so I think topNSlowQueries is better.

coocood · 2018-09-10T13:17:22Z

domain/topn_slow_query.go

+	close(q.ch)
+}
+
+func (q *topNSlowQuery) Push(info *slowQueryInfo) {


Why not implement the Heap interface in this type?
For the name confliction, we can change this Push to Add or Append.

tiancaiamao · 2018-09-10T16:02:24Z

PTAL @coocood

winkyao · 2018-09-11T03:11:50Z

executor/adapter.go

@@ -371,6 +372,13 @@ func (a *ExecStmt) logSlowQuery(txnTS uint64, succ bool) {
 		logutil.SlowQueryLogger.Warnf(
 			"[SLOW_QUERY] %vcost_time:%v %s succ:%v con:%v user:%s txn_start_ts:%v database:%v %v%vsql:%v",
 			internal, costTime, sessVars.StmtCtx.GetExecDetails(), succ, connID, user, txnTS, currentDB, tableIDs, indexIDs, sql)
+		if !sessVars.InRestrictedSQL {


Just log general sql? I prefer to keep two heap to log the general sql and internal sql.

Internal SQL are always the same, it will not give us too much information.

winkyao · 2018-09-11T03:18:39Z

domain/domain.go

+}
+
+func (do *Domain) topNSlowQueryLoop() {
+	defer do.wg.Done()


recover this goroutine.

winkyao · 2018-09-11T03:35:38Z

domain/topn_slow_query.go

+	}
+
+	// Rebuild the heap.
+	q.data = q.data[:idx]


You must use a lock to protect q.data, as long as you need to read the slice later.

We can do reading in the same goroutine.

I'll use copy on read, and they will be in one goroutine, no lock. @winkyao

winkyao · 2018-09-11T03:46:54Z

domain/topn_slow_query.go

+	q.data = append(q.data, x.(*slowQueryInfo))
+}
+
+func (q *topNSlowQueries) Pop() interface{} {


Pop can only return the minimum duration query, how can we implement topn, for example, the n is 30, and I wanna get top 3 query. and how can we just peek the heap?

Maybe a b-tree is better?

The read operation is not implemented in this PR.
Reading doesn't need to call Pop()

Read will not be a frequent operation, so we just copy on read.
Get top 3 query is easy, copy the origin heap, Pop Pop Pop.

tiancaiamao · 2018-09-11T05:48:12Z

PTAL @shenli @winkyao

coocood · 2018-09-11T05:59:20Z

LGTM

winkyao

LGTM

zhexuany

LGTM

zz-jason · 2018-09-11T06:59:23Z

domain/domain.go

@@ -329,6 +331,48 @@ func (do *Domain) Reload() error {
 	return nil
 }

+// LogTopNSlowQuery keeps topN recent slow queries in domain.
+func (do *Domain) LogTopNSlowQuery(sql string, start time.Time, duration time.Duration,


This function takes so many parameters, which makes it hard to read and maintain, could you extract a struct to store all the parameters and pass the struct as the parameter instead?

maybe slowQueryInfo is 🐶

how about exporting slowQueryInfo and use slowQueryInfo instead?

zz-jason · 2018-09-11T07:13:12Z

domain/topn_slow_query.go

+}
+
+func (h *slowQueryHeap) Len() int           { return len(h.data) }
+func (h *slowQueryHeap) Less(i, j int) bool { return h.data[i].duration < h.data[j].duration }


should it be h.data[i].duration > h.data[j].duration?

It's Less ... @zz-jason

use heap[len(heap)-1] to store the slowest query?

heap[0] is the fastest slow query, heap[len(heap)-1] may not be the slowest query.

zz-jason

has unresolved comments

tiancaiamao · 2018-09-11T07:39:02Z

PTAL @zz-jason

zz-jason · 2018-09-11T08:30:21Z

LGTM

zz-jason · 2018-09-11T08:30:43Z

/run-all-tests

zimulala · 2018-09-11T09:08:54Z

/run-all-tests

tiancaiamao · 2018-09-11T17:31:45Z

/run-all-tests

zz-jason · 2018-09-12T02:33:19Z

/run-all-tests

domain,executor: store topN slow query in domain

5f028d2

tiancaiamao added status/WIP sig/execution SIG execution labels Sep 8, 2018

tiancaiamao added 3 commits September 10, 2018 16:25

add test case and tiny fix

5f3878e

Merge branch 'master' into topn-slow-query

b2af9cb

fix CI

537e26d

tiancaiamao removed the status/WIP label Sep 10, 2018

coocood reviewed Sep 10, 2018

View reviewed changes

domain/topn_slow_query.go Outdated

}

}

func (q *topNSlowQuery) shiftUp(end int) {

Copy link

Member

coocood Sep 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

siftUp?

tiancaiamao added 2 commits September 10, 2018 20:25

fix typo

85d5f0d

use standard library for heap implementation

3b126bf

coocood reviewed Sep 10, 2018

View reviewed changes

address comment

5941733

winkyao reviewed Sep 11, 2018

View reviewed changes

domain/domain.go Outdated

}

func (do *Domain) topNSlowQueryLoop() {

defer do.wg.Done()

Copy link

Contributor

winkyao Sep 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recover this goroutine.

address comment

c388314

winkyao reviewed Sep 11, 2018

View reviewed changes

split into two heaps: user and interal

8dc40fb

winkyao previously approved these changes Sep 11, 2018

View reviewed changes

zhexuany previously approved these changes Sep 11, 2018

View reviewed changes

zz-jason reviewed Sep 11, 2018

View reviewed changes

zz-jason requested changes Sep 11, 2018

View reviewed changes

zz-jason added type/enhancement The issue or PR belongs to an enhancement. status/LGT2 Indicates that a PR has LGTM 2. labels Sep 11, 2018

tiancaiamao added 2 commits September 11, 2018 15:19

address comment

9075307

Merge branch 'master' into topn-slow-query

d88c674

tiancaiamao dismissed stale reviews from zhexuany and winkyao via d88c674 September 11, 2018 07:38

tiancaiamao force-pushed the topn-slow-query branch from c9db110 to d88c674 Compare September 11, 2018 07:38

zz-jason approved these changes Sep 11, 2018

View reviewed changes

Merge branch 'master' into topn-slow-query

94f4ab8

Merge branch 'master' into topn-slow-query

e01a1f4

tiancaiamao merged commit 6604e33 into pingcap:master Sep 12, 2018

tiancaiamao deleted the topn-slow-query branch September 12, 2018 03:02

tiancaiamao added the status/all tests passed label Sep 12, 2018

SunRunAway mentioned this pull request Dec 20, 2019

proposal: Support writing slow log in csv. #14170

Closed

domain,executor: store topN slow query in domain #7646

domain,executor: store topN slow query in domain #7646

Conversation

tiancaiamao commented Sep 8, 2018 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

shenli commented Sep 9, 2018

winkyao commented Sep 10, 2018

tiancaiamao commented Sep 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coocood commented Sep 10, 2018

tiancaiamao commented Sep 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiancaiamao commented Sep 10, 2018

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

winkyao Sep 11, 2018 • 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

tiancaiamao commented Sep 11, 2018

coocood commented Sep 11, 2018

winkyao left a comment

Choose a reason for hiding this comment

zhexuany 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

zz-jason Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

tiancaiamao commented Sep 11, 2018

zz-jason commented Sep 11, 2018

zz-jason commented Sep 11, 2018

zimulala commented Sep 11, 2018

tiancaiamao commented Sep 11, 2018

zz-jason commented Sep 12, 2018

tiancaiamao commented Sep 8, 2018 •

edited

Loading

winkyao Sep 11, 2018 •

edited

Loading

zz-jason Sep 11, 2018 •

edited

Loading

zz-jason Sep 11, 2018 •

edited

Loading