*: improve performance of show variables #5297

XuHuaiyu · 2017-12-04T07:30:09Z

fix #5126
PTAL @winkyao
Before this commit:
When run show variables, TiDB will autorun sql SELECT VARIABLE_VALUE FROM %s.%s WHERE VARIABLE_NAME="%s";
to fetch the value of every VARIABLE_NAME which
caused bad performance as #5126 described.

After commit:
We get all the data just once hence avoiding the time cost.

On my machine, for sql:

SHOW VARIABLES WHERE Variable_name ='language' OR Variable_name = 'net_write_timeout' OR Variable_name = 'interactive_timeout' OR Variable_name = 'wait_timeout' OR Variable_name = 'character_set_client' OR Variable_name = 'character_set_connection' OR Variable_name = 'character_set' OR Variable_name = 'character_set_server' OR Variable_name = 'tx_isolation' OR Variable_name = 'transaction_isolation' OR Variable_name = 'character_set_results' OR Variable_name = 'timezone' OR Variable_name = 'time_zone' OR Variable_name = 'system_time_zone' OR Variable_name = 'lower_case_table_names' OR Variable_name = 'max_allowed_packet' OR Variable_name = 'net_buffer_length' OR Variable_name = 'sql_mode' OR Variable_name = 'query_cache_type' OR Variable_name = 'query_cache_size' OR Variable_name = 'license' OR Variable_name = 'init_connect';

before: 0.59s
after:0.03s

winkyao · 2017-12-04T07:54:50Z

plan/planbuilder.go

@@ -599,6 +601,12 @@ func (b *planBuilder) buildShow(show *ast.ShowStmt) Plan {
 	for i, col := range p.schema.Columns {
 		col.Position = i
 	}
+	if show.Tp == ast.ShowVariables {


why not handle this in line 582?

This action should be after schema building.
So it is put after line601~604.

winkyao · 2017-12-04T07:57:27Z

plan/planbuilder.go

+	if b.err != nil {
+		return p
+	}
+	for i, cols := 0, ds.Schema().Columns; i < len(cols); i++ {


for _, cols := range ds.Schema().Columns

shenli · 2017-12-04T08:20:33Z

It is better to use a sql like SELECT VARIABLE_NAME, VARIABLE_VALUE FROM %s.%s WHERE VARIABLE_NAME="%s" or VARIABLE_NAME="%s" or ......;.
Using things in the executor package will cause problem for future work.

XuHuaiyu · 2017-12-04T09:27:02Z

PTAL @shenli
This implementation cost ~~0.07s~~ 0.03s on my machine when run

SHOW VARIABLES WHERE Variable_name ='language' OR Variable_name = 'net_write_timeout' OR Variable_name = 'interactive_timeout' OR Variable_name = 'wait_timeout' OR Variable_name = 'character_set_client' OR Variable_name = 'character_set_connection' OR Variable_name = 'character_set' OR Variable_name = 'character_set_server' OR Variable_name = 'tx_isolation' OR Variable_name = 'transaction_isolation' OR Variable_name = 'character_set_results' OR Variable_name = 'timezone' OR Variable_name = 'time_zone' OR Variable_name = 'system_time_zone' OR Variable_name = 'lower_case_table_names' OR Variable_name = 'max_allowed_packet' OR Variable_name = 'net_buffer_length' OR Variable_name = 'sql_mode' OR Variable_name = 'query_cache_type' OR Variable_name = 'query_cache_size' OR Variable_name = 'license' OR Variable_name = 'init_connect';

~~It's slower than the last implementation since the last one can push the selection down to datasource.~~

shenli · 2017-12-04T09:35:40Z

@XuHuaiyu You can add a hint for this query which could reduce the cost of optimizer.

XuHuaiyu · 2017-12-04T10:27:02Z

PTAL @winkyao @coocood

coocood · 2017-12-04T11:04:19Z

session.go

+	if s.Value(context.Initing) != nil {
+		return nil, nil
+	}
+	sql := `SELECT VARIABLE_NAME, VARIABLE_VALUE FROM %s.%s WHERE VARIABLE_NAME in (`


It will be even faster if we remove the IN condition.

GetAllSysVars does not need in condition, select all is OK.

:q!

mccxj · 2017-12-04T11:14:37Z

session.go

+	if s.Value(context.Initing) != nil {
+		return nil, nil
+	}
+	sql := `SELECT VARIABLE_NAME, VARIABLE_VALUE FROM %s.%s WHERE VARIABLE_NAME in (`


GetAllSysVars does not need in condition, select all is OK.

mccxj · 2017-12-04T11:49:49Z

executor/show.go

@@ -87,7 +87,7 @@ func (e *ShowExec) Next(goCtx goctx.Context) (Row, error) {
 	return row, nil
 }

-func (e *ShowExec) fetchAll() error {
+func (e *ShowExec) fetchAll(goCtx goctx.Context) error {


goctx is useless?

XuHuaiyu · 2017-12-04T12:32:59Z

PTAL @coocood @winkyao @mccxj

coocood · 2017-12-04T13:30:03Z

executor/show.go

 		}
 		if err != nil {
 			return errors.Trace(err)
 		}
+		if !ok {
+			value, ok = systemVars[v.Name]


This will do.

if !ok { value = v.Value }

mccxj · 2017-12-04T13:25:51Z

executor/show.go

+		if !ok {
+			value, ok = systemVars[v.Name]
+			if !ok {
+				sv, ok2 := variable.SysVars[v.Name]


ok2 is always true, because v was fetched from SysVars.

mccxj · 2017-12-04T13:29:04Z

executor/show.go

+			value, ok = systemVars[v.Name]
+			if !ok {
+				sv, ok2 := variable.SysVars[v.Name]
+				isUninitializedGlobalVariable := ok2 && sv.Scope|variable.ScopeGlobal > 0


as an early bug. "sv.Scope|variable.ScopeGlobal > 0" is always true.

shenli · 2017-12-04T13:56:58Z

session.go

+	if s.Value(context.Initing) != nil {
+		return nil, nil
+	}
+	sql := `SELECT VARIABLE_NAME, VARIABLE_VALUE FROM %s.%s;`


Why should we load all global variables from tikv?

This makes no difference for performance,
since SysVars hold almost all the variable names(https://github.com/pingcap/tidb/blob/master/sessionctx/variable/sysvar.go#L99).

I care about the performance, not the memory consuming. We only need to load the variables that in the show variable statement.

:q!

coocood · 2017-12-04T15:40:50Z

sessionctx/varsutil/varsutil.go

 	}
 	if sysVar.Scope == variable.ScopeSession {
-		return "", variable.ErrIncorrectScope
+		return "", false, variable.ErrIncorrectScope


We can move this error to GetGlobalSystemVar.

Conflicts: sessionctx/varsutil/varsutil.go

XuHuaiyu · 2017-12-05T07:26:31Z

PTAL @coocood @winkyao

winkyao · 2017-12-05T07:54:22Z

executor/show.go

 		} else {
-			value, err = varsutil.GetGlobalSystemVar(sessionVars, v.Name)
+			value, ok, err = varsutil.GetScopeNoneSystemVar(v.Name)


add some comment here. about session scope, global scope and none scope, it is confused.

winkyao · 2017-12-05T07:59:18Z

reset LGTM

coocood · 2017-12-05T07:59:34Z

LGTM

XuHuaiyu · 2017-12-05T08:15:14Z

PTAL @winkyao

winkyao

LGTM

mccxj

LGTM

* show: fix variable scope error (#5291) * *: improve performance of show variables (#5297) * *: session variable value should not be modified when get global variable (#5317) * fix ci * ddl,expression: make leak test more stable (#4895)

*: improve performance of show variables

e16c54e

XuHuaiyu mentioned this pull request Dec 4, 2017

show: fix variable scope error #5291

Merged

winkyao reviewed Dec 4, 2017

View reviewed changes

address comment

1e04fa8

XuHuaiyu mentioned this pull request Dec 4, 2017

Improve show variables performance. #5126

Closed

coocood reviewed Dec 4, 2017

View reviewed changes

XuHuaiyu added 2 commits December 4, 2017 19:27

Merge branch 'master' of https://github.com/pingcap/tidb into show

a993b7a

:q!

address commnet

4a8e3a6

mccxj reviewed Dec 4, 2017

View reviewed changes

address comment

91ddd51

coocood reviewed Dec 4, 2017

View reviewed changes

mccxj reviewed Dec 4, 2017

View reviewed changes

shenli reviewed Dec 4, 2017

View reviewed changes

XuHuaiyu added 2 commits December 4, 2017 23:06

address comment

f231d94

Merge branch 'master' of https://github.com/pingcap/tidb into show

d73a664

:q!

XuHuaiyu force-pushed the show branch from adcf186 to d73a664 Compare December 4, 2017 15:10

coocood reviewed Dec 4, 2017

View reviewed changes

XuHuaiyu added 3 commits December 5, 2017 13:54

address comment

3ae328f

Merge branch 'master' of https://github.com/pingcap/tidb into show

821f2e6

Conflicts: sessionctx/varsutil/varsutil.go

fix ci

5c3e77a

XuHuaiyu force-pushed the show branch from dea9f6f to 5c3e77a Compare December 5, 2017 06:54

address commnet

fb9e3f5

winkyao reviewed Dec 5, 2017

View reviewed changes

address comment

38dcd8a

coocood added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 5, 2017

Merge branch 'master' into show

a590ffa

winkyao approved these changes Dec 5, 2017

View reviewed changes

mccxj approved these changes Dec 5, 2017

View reviewed changes

XuHuaiyu added 2 commits December 5, 2017 18:00

Merge branch 'master' into show

c89897a

Merge branch 'master' into show

ddf9eea

XuHuaiyu added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 5, 2017

Merge branch 'master' into show

0af2e84

coocood merged commit 714b68c into pingcap:master Dec 5, 2017

XuHuaiyu mentioned this pull request Dec 11, 2017

*: improve performance of show variables #5363

Merged

XuHuaiyu added a commit to XuHuaiyu/tidb that referenced this pull request Dec 11, 2017

*: improve performance of show variables (pingcap#5297)

769a3b0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: improve performance of show variables #5297

*: improve performance of show variables #5297

XuHuaiyu commented Dec 4, 2017 •

edited

Loading

winkyao Dec 4, 2017

XuHuaiyu Dec 4, 2017

winkyao Dec 4, 2017

shenli commented Dec 4, 2017

XuHuaiyu commented Dec 4, 2017 •

edited

Loading

shenli commented Dec 4, 2017

XuHuaiyu commented Dec 4, 2017

coocood Dec 4, 2017

mccxj Dec 4, 2017

mccxj Dec 4, 2017

mccxj Dec 4, 2017

XuHuaiyu commented Dec 4, 2017

coocood Dec 4, 2017

mccxj Dec 4, 2017

mccxj Dec 4, 2017

shenli Dec 4, 2017

XuHuaiyu Dec 4, 2017 •

edited

Loading

shenli Dec 4, 2017

coocood Dec 4, 2017

XuHuaiyu commented Dec 5, 2017

winkyao Dec 5, 2017

winkyao commented Dec 5, 2017

coocood commented Dec 5, 2017

XuHuaiyu commented Dec 5, 2017

winkyao left a comment

mccxj left a comment

*: improve performance of show variables #5297

*: improve performance of show variables #5297

Conversation

XuHuaiyu commented Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shenli commented Dec 4, 2017

XuHuaiyu commented Dec 4, 2017 • edited Loading

shenli commented Dec 4, 2017

XuHuaiyu commented Dec 4, 2017

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

XuHuaiyu commented Dec 4, 2017

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

XuHuaiyu Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XuHuaiyu commented Dec 5, 2017

Choose a reason for hiding this comment

winkyao commented Dec 5, 2017

coocood commented Dec 5, 2017

XuHuaiyu commented Dec 5, 2017

winkyao left a comment

Choose a reason for hiding this comment

mccxj left a comment

Choose a reason for hiding this comment

XuHuaiyu commented Dec 4, 2017 •

edited

Loading

XuHuaiyu commented Dec 4, 2017 •

edited

Loading

XuHuaiyu Dec 4, 2017 •

edited

Loading