executor: refine `explain analyze` #7888

lysu · 2018-10-12T17:37:30Z

What problem does this PR solve?

this PR address the contiune comments in #7827

the mainly change are:

protect runtimeStats with mutex, explain analyze is infrequently operation but executor maybe uder concurrent execution.
forbidden collect stats of the IndexLookupExecutor that owned by table worker, it seems no other executor like this one - -
refactor explainBuilder/explainExec, push render data & analyze logic down to explainExec
rename some variable/struct

What is changed and how it works?

RT

Check List

Tests

Old test

Code changes

impl refactor

Side effects

no

Related changes

Need to cherry-pick to 2.1

This change is

lysu · 2018-10-12T17:37:57Z

/run-all-tests

&&

PTAL @zz-jason

zz-jason · 2018-10-15T10:24:12Z

util/execdetails/execdetails.go

@@ -69,43 +72,35 @@ type RuntimeStat struct {
 }

 // NewRuntimeStats creates new executor collector.
-func NewRuntimeStats() RuntimeStats {
-	return RuntimeStats(make(map[string]*RuntimeStat))
+func NewRuntimeStats() *RuntimeStatsColl {


how about: s/NewRuntimeStats/NewRuntimeStatsColl/

zz-jason · 2018-10-15T10:24:56Z

util/execdetails/execdetails.go

 }

 // GetRuntimeStat gets execStat for a executor.
-func (e RuntimeStats) GetRuntimeStat(planID string) *RuntimeStat {
+func (e *RuntimeStatsColl) GetRuntimeStat(planID string) *RuntimeStats {


how about s/GetRuntimeStat/Get/

zz-jason · 2018-10-15T10:25:29Z

util/execdetails/execdetails.go

 }

 // GetRuntimeStat gets execStat for a executor.
-func (e RuntimeStats) GetRuntimeStat(planID string) *RuntimeStat {
+func (e *RuntimeStatsColl) GetRuntimeStat(planID string) *RuntimeStats {
 	if e == nil {


this check can be removed?

zz-jason · 2018-10-15T10:34:13Z

executor/distsql.go

@@ -489,7 +488,9 @@ func (e *IndexLookUpExecutor) buildTableReader(ctx context.Context, handles []in
 		feedback:        statistics.NewQueryFeedback(0, nil, 0, false),
 		corColInFilter:  e.corColInTblSide,
 		plans:           e.tblPlans,
-	}, handles)
+	}
+	tableReaderExec.statsEnable = false


should be set according to the statement type, i.e.:

ctx.GetSessionVars().StmtCtx.RuntimeStatsColl != nil

zz-jason · 2018-10-15T10:57:38Z

executor/executor.go

@@ -78,7 +78,8 @@ type baseExecutor struct {
 	maxChunkSize  int
 	children      []Executor
 	retFieldTypes []*types.FieldType
-	runtimeStat   *execdetails.RuntimeStat
+	statsEnable   bool
+	_runtimeStats *execdetails.RuntimeStats


I think the old way is better. And to eliminate misunderstanding that e.runtimeStats will always be set, in function newBaseExecutor() we can:

if ctx.GetSessionVars().StmtCtx.RuntimeStatsColl != nil { e.runtimeStats = e.ctx.GetSessionVars().StmtCtx.RuntimeStatsColl.GetRuntimeStat(e.id) }

executor/distsql.go

zz-jason

LGTM

zz-jason · 2018-10-16T04:59:39Z

@XuHuaiyu @lamxTyler PTAL

alivxxx · 2018-10-16T09:10:13Z

util/execdetails/execdetails.go

 	if e == nil {
 		return ""
 	}
-	return fmt.Sprintf("time:%f, loops:%d, rows:%d", time.Duration(e.consume).Seconds()*1e3, e.loop, e.rows)
+	return fmt.Sprintf("time:%fms, loops:%d, rows:%d", time.Duration(e.consume).Seconds()*1e3, e.loop, e.rows)


May be we can use %v to let the system libaray handle the time unit.

IMHO, fixed ms is better.

If it takes too long, we need to mentally parse it to something like min or hour?

it's not normal has min or hour executue time and more 000000 is ok for me too 🤣

how do u think about ~? @zz-jason @XuHuaiyu

DONE~~~PTAL @lamxTyler 😆

make result render down to exec stage.

alivxxx

LGTM

bugfix fixed pingcap#7518 expression: MySQL compatible current_user function (pingcap#7801) plan: propagate constant over outer join (pingcap#7794) - extract `outerCol = const` from join conditions and filter conditions, substitute `outerCol` in join conditions with `const`; - extract `outerCol = innerCol` from join conditions, derive new join conditions based on this column equal condition and `outerCol` related expressions in join conditions and filter conditions; util/timeutil: fix data race caused by forgetting set stats lease to 0 (pingcap#7901) stats: handle ddl event for partition table (pingcap#7903) plan: implement Operand and Pattern of cascades planner. (pingcap#7910) planner: not convert to TableDual if empty range is derived from deferred constants (pingcap#7808) plan: move projEliminate behind aggEliminate (pingcap#7909) admin: fix admin check table bug of byte compare (pingcap#7887) * admin: remove reflect deepEqual stats: fix panic caused by empty histogram (pingcap#7912) plan: fix panic caused by empty schema of LogicalTableDual (pingcap#7906) * fix drop view if exist error (pingcap#7833) executor: refine `explain analyze` (pingcap#7888) executor: add an variable to compatible with MySQL insert for OGG (pingcap#7863) expression: maintain `DeferredExpr` in aggressive constant folding. (pingcap#7915) stats: fix histogram boundaries overflow error (pingcap#7883) ddl:support the definition of `null` change to `not null` using `alter table` (pingcap#7771) * ddl:support the definition of null change to not null using alter table ddl: add check when create table with foreign key. (pingcap#7885) * ddl: add check when create table with foreign key planner: eliminate if null on non null column (pingcap#7924) executor: fix a bug in point get (pingcap#7934) planner, executor: refine ColumnPrune for LogicalUnionAll (pingcap#7930) executor: fix panic when limit is too large (pingcap#7936) ddl: add TiDB version to metrics (pingcap#7902) stats: limit the length of sample values (pingcap#7931) vendor: update tipb (pingcap#7893) planner: support the Group and GroupExpr for the cascades planner (pingcap#7917) store/tikv: log more information when other err occurs (pingcap#7948) types: fix date time parse (pingcap#7933) ddl: just print error message when ddl job is normal to calcel, to eliminate noisy log (pingcap#7875) stats: update delta info for partition table (pingcap#7947) explaintest: add explain test for partition pruning (pingcap#7505) util: move disjoint set to util package (pingcap#7950) util: add PreAlloc4Row and Insert for Chunk and List (pingcap#7916) executor: add the slow log for commit (pingcap#7951) expression: add builtin json_keys (pingcap#7776) privilege: add USAGE in `show grants` for mysql compatibility (pingcap#7955) ddl: fix invailid ddl job panic (pingcap#7940) *: move ast.NewValueExpr to standalone parser_driver package (pingcap#7952) Make the ast package get rid of the dependency of types.Datum server: allow cors http request (pingcap#7939) *: move `Statement` and `RecordSet` from ast to sqlexec package (pingcap#7970) pr suggestion update executor/aggfuncs: split unit tests to corresponding file (pingcap#7993) store/tikv: fix typo (pingcap#7990) executor, planner: clone proj schema for different children in buildProj4Union (pingcap#7999) executor: let information_schema be the first database in ShowDatabases (pingcap#7938) stats: use local feedback for partition table (pingcap#7963) executor: add unit test for aggfuncs (pingcap#7966) server: add log for binary execute statement (pingcap#7987) admin: refine admin check decoder (pingcap#7862) executor: improve wide table insert & update performance (pingcap#7935) ddl: fix reassigned partition id in `truncate table` does not take effect (pingcap#7919) fix reassigned partition id in truncate table does not take effect add changelog for 2.1.0 rc4 (pingcap#8020) *: make parser package dependency as small as possible (pingcap#7989) parser: support `:=` in the `set` syntax (pingcap#8018) According to MySQL document, `set` use the = assignment operator, but the := assignment operator is also permitted stats: garbage collect stats for partition table (pingcap#7962) docs: add the proposal for the column pool (pingcap#7988) expression: refine built-in func truncate to support uint arg (pingcap#8000) stats: support show stats for partition table (pingcap#8023) stats: update error rate for partition table (pingcap#8022) stats: fix estimation for out of range point queries (pingcap#8015) *: move parser to a separate repository (pingcap#8036) executor: fix wrong result when index join on union scan. (pingcap#8031) Do not modify Plan of dataReaderBuilder directly, because it would impact next batch of outer rows, as well as other concurrent inner workers. Instead, build a local child builder to store the child plan. planner: fix a panic of a cached prepared statement with IndexScan (pingcap#8017) *: fix the issue of executing DDL after executing SQL failure in txn (pingcap#8044) * ddl, executor: fix the issue of executing DDL after executing SQL failure in txn add unit test remove debug info add like evaluator case sensitive test ddl, domain: make schema correct after canceling jobs (pingcap#7997) unit test fix code format proposal: maintaining histograms in plan. (pingcap#7605) support _tidb_rowid for table scan range (pingcap#8047) var rename fix

lysu added sig/execution SIG execution status/all tests passed labels Oct 12, 2018

zz-jason reviewed Oct 15, 2018

View reviewed changes

executor/distsql.go Show resolved Hide resolved

zz-jason reviewed Oct 15, 2018

View reviewed changes

lysu added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 15, 2018

alivxxx reviewed Oct 16, 2018

View reviewed changes

lysu added 5 commits October 16, 2018 18:53

executor: deduce runtimeStat alloc

478bd48

executor: refactor explain

b36596b

make result render down to exec stage.

address comment

d414055

address comment

f4a5077

address comment

3cfd0a9

alivxxx approved these changes Oct 16, 2018

View reviewed changes

alivxxx added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 16, 2018

alivxxx and others added 2 commits October 16, 2018 19:05

Merge branch 'master' into fix-explain-analyze

ef47851

Merge branch 'master' into fix-explain-analyze

5b40c38

alivxxx merged commit 458c0d1 into pingcap:master Oct 16, 2018

lysu mentioned this pull request Oct 17, 2018

*: make explain support explain anaylze (#7827)(#7888) #7925

Merged

ngaut pushed a commit that referenced this pull request Oct 18, 2018

*: make explain support explain anaylze (#7827)(#7888) (#7925)

52d5ee2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: refine `explain analyze` #7888

executor: refine `explain analyze` #7888

lysu commented Oct 12, 2018 •

edited

Loading

lysu commented Oct 12, 2018

zz-jason Oct 15, 2018

zz-jason Oct 15, 2018

zz-jason Oct 15, 2018

zz-jason Oct 15, 2018

zz-jason Oct 15, 2018

lysu Oct 15, 2018

zz-jason left a comment

zz-jason commented Oct 16, 2018

alivxxx Oct 16, 2018

lysu Oct 16, 2018

alivxxx Oct 16, 2018

lysu Oct 16, 2018

lysu Oct 16, 2018

alivxxx left a comment

executor: refine explain analyze #7888

executor: refine explain analyze #7888

Conversation

lysu commented Oct 12, 2018 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

lysu commented Oct 12, 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

zz-jason left a comment

Choose a reason for hiding this comment

zz-jason commented Oct 16, 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

alivxxx left a comment

Choose a reason for hiding this comment

executor: refine `explain analyze` #7888

executor: refine `explain analyze` #7888

lysu commented Oct 12, 2018 •

edited

Loading