server: add http api to get some info of sub-optimal query #10717

winoros · 2019-06-05T06:36:31Z

What problem does this PR solve?

add HTTP api /sub-optimal-plan?pprof_time=&timeout=
to get some information of one SQL.
If we don't set pprof_time. We'll just get explain result. Otherwise we get explain analyze result.

What is changed and how it works?

Extract table names from SQL.
Dump the statistics of the tables.
Use show create table to get the schema of them.
Use Explain/Explain analyze to the result.
If we run explain analyze, we'll run pprof together to get CPU profile.

Check List

Tests

Manual test (use curl -X POST ... test it).

Related changes

Need to update the documentation
Need to be included in the release note

winoros · 2019-06-05T06:37:34Z

server/http_status.go

-		terror.Log(err)
-	}
-
-	sleep := func(w http.ResponseWriter, d time.Duration) {


// Deprecated: the CloseNotifier interface predates Go's context package. // New code should use Request.Context instead.

winoros · 2019-06-05T06:38:34Z

server/sql_info_fetcher.go

+		jsonTbl, err := sh.getStatsForTable(pair)
+		if err != nil {
+			terror.Log(err)
+			continue


Should we continue or just throw error out?

can we return a message to indicate the failure of dumping stats for this table?

write a file like dbname.tblname.stats.err?

looks OK to me

codecov · 2019-06-05T06:45:24Z

Codecov Report

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

@@             Coverage Diff             @@
##             master     #10717   +/-   ##
===========================================
  Coverage   80.9431%   80.9431%           
===========================================
  Files           420        420           
  Lines         89506      89506           
===========================================
  Hits          72449      72449           
  Misses        11829      11829           
  Partials       5228       5228

zz-jason · 2019-06-05T06:59:54Z

server/sql_info_fetcher.go

+func (sh *sqlInfoFetcher) zipInfoForSQL(w http.ResponseWriter, r *http.Request) {
+	reqCtx := r.Context()
+	sql := r.FormValue("sql")
+	pprofTimeString := r.FormValue("pprof_time")


how about profile_sec?

zz-jason · 2019-06-05T08:25:23Z

server/sql_info_fetcher.go

+		return
+	}
+	if pprofTime < 5 {
+		serveError(w, http.StatusBadRequest, "pprof time is too short")


I think a best practice for this kind of error message is:

point out the invalid value

point out the valid value range

Should we return here if it's an invalid value?

zz-jason · 2019-06-05T08:31:33Z

server/sql_info_fetcher.go

+	}
+	pairs, err := sh.extractTableNames(sql)
+	if err != nil {
+		serveError(w, http.StatusBadRequest, fmt.Sprintf("invalid SQL text, err: %v", err))


how about "failed to extract table names, sql: %v"

But the sql is something given by the user.

zz-jason · 2019-06-05T08:34:05Z

server/sql_info_fetcher.go

+		jsonTbl, err := sh.getStatsForTable(pair)
+		if err != nil {
+			terror.Log(err)
+			continue


can we return a message to indicate the failure of dumping stats for this table?

server/sql_info_fetcher.go

qw4990 · 2019-06-10T02:56:26Z

server/sql_info_fetcher.go

+	for pair := range pairs {
+		err = sh.getShowCreateTable(pair, zw)
+		if err != nil {
+			serveError(w, http.StatusInternalServerError, fmt.Sprintf("get schema for %v.%v failed", pair.DBName, pair.TableName))


Is it ok that we write error messages and zip formatted data to w at the same time?

In fact, it's not very ok. The following is the curl result when error arisen.

➜ tidb git:(http-fetch-sql-info) ✗ curl -X POST http://127.0.0.1:10080/debug/sub-optimal-plan -d "sql=select%20*%20from%20t&current_db=test" execute SQL failed, err: [planner:1046]No database selected PK test.t.json��Aj�0��Z ��i�� [iDe+X��"=C��a��[h�Ϋ?�� V4#��陉�6��Y��Ƣ�}��2 u�B��P�:��Ω��t�⧤��~��ܣ&�Ѭ��ɕ�A�E�t�wc�y�k�m�zl6��aw|�yC��H�؆"v�F�+��ן�7?j\��u�O��P� "f��Ӡa\��T XQtest.t.schema.txt*�tru qqt�qUH(IP��RPH�LP(K,J�H,�02�Tpqus � Q� ��Tp�s��s��wq�K:{8��ؖ��Y�&�(8��8��I�y\��P(%��ip� "f XQ test.t.json(%��ipCtest.t.schema.txtPKx�%

But the api debug/zip did the same thing. #9651
Or we may need to write to a BytesBuffer first then write to ResponseWriter

OK... for simplifying, we can leave it as a further optimization and do it in another PR and please add a TODO here.

zz-jason · 2019-06-10T03:43:59Z

server/sql_info_fetcher.go

+	timeoutString := r.FormValue("timeout")
+	curDB := strings.ToLower(r.FormValue("current_db"))
+	if curDB != "" {
+		_, err = sh.s.Execute(reqCtx, fmt.Sprintf("use %v", curDB))


Suggested change

_, err = sh.s.Execute(reqCtx, fmt.Sprintf("use %v", curDB))

_, err = sh.s.Execute(reqCtx, "use " + curDB)

zz-jason · 2019-06-10T03:45:01Z

server/sql_info_fetcher.go

+	if curDB != "" {
+		_, err = sh.s.Execute(reqCtx, fmt.Sprintf("use %v", curDB))
+		if err != nil {
+			serveError(w, http.StatusInternalServerError, fmt.Sprintf("use database failed, err: %v", err))


Suggested change

serveError(w, http.StatusInternalServerError, fmt.Sprintf("use database failed, err: %v", err))

serveError(w, http.StatusInternalServerError, fmt.Sprintf("use database %v failed: %v", curDB , err))

zz-jason · 2019-06-10T03:46:17Z

server/sql_info_fetcher.go

+	if pprofTimeString != "" {
+		pprofTime, err = strconv.Atoi(pprofTimeString)
+	}
+	if err != nil {


check err in the above if statement

zz-jason · 2019-06-10T03:49:18Z

server/sql_info_fetcher.go

+	if timeoutString != "" {
+		timeout, err = strconv.Atoi(timeoutString)
+	}
+	if err != nil {


zz-jason · 2019-06-10T04:36:58Z

server/sql_info_fetcher.go

+		jsonTbl, err := sh.getStatsForTable(pair)
+		if err != nil {
+			terror.Log(err)
+			continue


looks OK to me

…into http-fetch-sql-info

alivxxx · 2019-06-11T08:59:46Z

server/sql_info_fetcher.go

+		curDB: curDB,
+		names: make(map[tableNamePair]struct{}),
+	}
+	for _, stmt := range stmts {


So we allow multiple statements? Will it cause problems if we use explain sql?

zz-jason

LGTM

alivxxx

LGTM

alivxxx · 2019-06-24T06:31:50Z

/run-all-tests

…0717)

server: add http api to get some info of sub-optimal query

a763b8a

winoros added type/enhancement The issue or PR belongs to an enhancement. type/usability labels Jun 5, 2019

winoros requested review from zz-jason, qw4990 and alivxxx June 5, 2019 06:36

winoros commented Jun 5, 2019

View reviewed changes

winoros force-pushed the http-fetch-sql-info branch from 5e5f7c4 to 0b32e29 Compare June 5, 2019 06:51

winoros added 2 commits June 5, 2019 14:52

fix make check

0b32e29

tiny change on file name

d2d94a5

zz-jason reviewed Jun 5, 2019

View reviewed changes

alivxxx reviewed Jun 5, 2019

View reviewed changes

server/sql_info_fetcher.go Outdated Show resolved Hide resolved

server/sql_info_fetcher.go Show resolved Hide resolved

address comments

68e99ef

winoros added the priority/P1 The issue has P1 priority. label Jun 6, 2019

Merge branch 'master' into http-fetch-sql-info

85a4bd8

qw4990 reviewed Jun 10, 2019

View reviewed changes

server/sql_info_fetcher.go Show resolved Hide resolved

qw4990 reviewed Jun 10, 2019

View reviewed changes

zz-jason reviewed Jun 10, 2019

View reviewed changes

winoros added 2 commits June 10, 2019 19:40

address comments

11b99a2

Merge branch 'http-fetch-sql-info' of https://github.com/winoros/tidb …

6cb8777

…into http-fetch-sql-info

alivxxx reviewed Jun 11, 2019

View reviewed changes

winoros added 3 commits June 24, 2019 11:28

address comments

6238feb

Merge branch 'master' into http-fetch-sql-info

e0aef2f

fix build

c152e7e

winoros requested review from alivxxx, qw4990 and zz-jason June 24, 2019 03:40

zz-jason reviewed Jun 24, 2019

View reviewed changes

zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 24, 2019

alivxxx approved these changes Jun 24, 2019

View reviewed changes

Merge branch 'master' into http-fetch-sql-info

65f5c09

Merge branch 'master' into http-fetch-sql-info

258ec5c

winoros merged commit 91ab534 into pingcap:master Jun 24, 2019

winoros deleted the http-fetch-sql-info branch June 24, 2019 08:59

winoros added a commit to winoros/tidb that referenced this pull request Jun 24, 2019

server: add http api to get some info of sub-optimal query (pingcap#1…

1d46410

…0717)

winoros mentioned this pull request Jun 24, 2019

server: add http api to get some info of sub-optimal query (#10717) #10925

Merged

Deardrops mentioned this pull request Jan 2, 2020

server: add test for /debug/sub-optimal-plan HTTP API #14302

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: add http api to get some info of sub-optimal query #10717

server: add http api to get some info of sub-optimal query #10717

winoros commented Jun 5, 2019

winoros Jun 5, 2019

winoros Jun 5, 2019

zz-jason Jun 5, 2019

winoros Jun 6, 2019

zz-jason Jun 10, 2019

codecov bot commented Jun 5, 2019 •

edited

Loading

zz-jason Jun 5, 2019

zz-jason Jun 5, 2019

qw4990 Jun 10, 2019

zz-jason Jun 5, 2019

winoros Jun 6, 2019

zz-jason Jun 5, 2019

qw4990 Jun 10, 2019

winoros Jun 10, 2019 •

edited

Loading

qw4990 Jun 10, 2019 •

edited

Loading

zz-jason Jun 10, 2019

zz-jason Jun 10, 2019

zz-jason Jun 10, 2019

zz-jason Jun 10, 2019

zz-jason Jun 10, 2019

alivxxx Jun 11, 2019

zz-jason left a comment

alivxxx left a comment

alivxxx commented Jun 24, 2019

	_, err = sh.s.Execute(reqCtx, fmt.Sprintf("use %v", curDB))
	_, err = sh.s.Execute(reqCtx, "use " + curDB)

	serveError(w, http.StatusInternalServerError, fmt.Sprintf("use database failed, err: %v", err))
	serveError(w, http.StatusInternalServerError, fmt.Sprintf("use database %v failed: %v", curDB , err))

server: add http api to get some info of sub-optimal query #10717

server: add http api to get some info of sub-optimal query #10717

Conversation

winoros commented Jun 5, 2019

What problem does this PR solve?

What is changed and how it works?

Check List

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

codecov bot commented Jun 5, 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

winoros Jun 10, 2019 • edited Loading

Choose a reason for hiding this comment

qw4990 Jun 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

zz-jason left a comment

Choose a reason for hiding this comment

alivxxx left a comment

Choose a reason for hiding this comment

alivxxx commented Jun 24, 2019

codecov bot commented Jun 5, 2019 •

edited

Loading

winoros Jun 10, 2019 •

edited

Loading

qw4990 Jun 10, 2019 •

edited

Loading