execution: support explain analyze in mpp execution. #22053

windtalker · 2020-12-28T08:24:14Z

What problem does this PR solve?

Issue Number: close pingcap/tiflash#1288

Problem Summary:
Currently, explain analyze does not show cop executor's execution summaries when in mpp execution mode

What is changed and how it works?

Proposal: xxx

What's Changed:

How it Works:

Collect cop executor execution summaries in mpp mode.

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
Need to cherry-pick to the release branch

Check List

Tests

Manual test (add detailed scripts or steps below)

Side effects

Performance regression
- Consumes more CPU
- Consumes more MEM
Breaking backward compatibility

Release note

No release note

sre-bot · 2020-12-28T08:24:25Z

Please follow PR Title Format:

pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

*: what's changed

…lain_info

windtalker · 2020-12-29T02:41:39Z

/run-all-tests

fzhedu · 2020-12-29T07:30:53Z

util/execdetails/execdetails_test.go

-	if stats.GetCopStats(aggID).String() != "tikv_task:{proc max:4ns, min:3ns, p80:4ns, p95:4ns, iters:7, tasks:2}" {
-		t.Fatal("agg")
+	if stats.GetCopStats(aggID).String() != "tikv_task:{proc max:4ns, min:3ns, p80:4ns, p95:4ns, iters:7, tasks:2, concurrency:2}" {
+		t.Fatalf("agg cop stats string is not expect, got: %v", stats.GetCopStats(aggID).String())


more common way is "as expected"

util/execdetails/execdetails.go

hanfei1991

LGTM

…lain_info

fzhedu · 2021-01-05T08:12:28Z

/run-tics-test

fzhedu · 2021-01-05T08:16:58Z

executor/explainfor_test.go

@@ -112,7 +112,7 @@ func (s *testSerialSuite) TestExplainFor(c *C) {
 		}
 		c.Assert(buf.String(), Matches, ""+
 			"TableReader_5 10000.00 0 root  time:.*, loops:1, cop_task: {num:.*, max:.*, proc_keys: 0, rpc_num: 1, rpc_time:.*} data:TableFullScan_4 N/A N/A\n"+
-			"└─TableFullScan_4 10000.00 0 cop.* table:t1 tikv_task:{time:.*, loops:0}, scan_detail:.* keep order:false, stats:pseudo N/A N/A")
+			"└─TableFullScan_4 10000.00 0 cop.* table:t1 tikv_task:{time:.*, loops:0, concurrency:1}, scan_detail:.* keep order:false, stats:pseudo N/A N/A")


tikv_task?
should tikv tasks show concurrency?

By default, every TiKV task has concurrency 1, I think it is at least harmless to show it for TiKV tasks, user can just ignore it.

you should ask the related PM.

I think keeping tikv's result still is better. No changes is better than harmless changes.

For TiKV, I will print threads:N/A instead

windtalker · 2021-01-07T02:06:05Z

/run-all-tests

fzhedu · 2021-01-07T02:39:51Z

util/execdetails/execdetails_test.go

 	return &tipb.ExecutorExecutionSummary{TimeProcessedNs: &TimeProcessedNs, NumProducedRows: &NumProducedRows,
-		NumIterations: &NumIterations, ExecutorId: &ExecutorID, XXX_unrecognized: nil}
+		NumIterations: &NumIterations, Concurrency: &Concurrency, ExecutorId: &ExecutorID, XXX_unrecognized: nil}


why here show concurrency?

For single cop/mpp task, this field is the running concurrency of the executor.

fzhedu · 2021-01-07T02:41:07Z

util/execdetails/execdetails.go

 			consume: int64(*summary.TimeProcessedNs),
-			rows:    int64(*summary.NumProducedRows)})
+			rows:    int64(*summary.NumProducedRows)},
+			threads:   int32(summary.GetConcurrency()),


getConcurrency() -> getThreads()?

getConcurrency is a method generated by protobuf

fzhedu · 2021-01-07T02:42:31Z

store/tikv/mpp.go

+	for backoff := range bo.backoffTimes {
+		backoffName := backoff.String()
+		resp.detail.BackoffTimes[backoffName] = bo.backoffTimes[backoff]
+		resp.detail.BackoffSleep[backoffName] = time.Duration(bo.backoffSleepMS[backoff]) * time.Millisecond


Because time.Duration(bo.backoffSleepMS[backoff]) is Nanosecond based.

fzhedu

LGTM

fzhedu · 2021-01-07T05:22:50Z

/LGTM

fzhedu · 2021-01-07T05:23:02Z

/merge

ti-srebot · 2021-01-07T05:23:06Z

Your auto merge job has been accepted, waiting for:

22165

ti-srebot · 2021-01-07T05:30:41Z

/run-all-tests

ti-srebot · 2021-01-07T05:33:42Z

@windtalker merge failed.

windtalker · 2021-01-07T05:59:14Z

/run-e2e-test

windtalker · 2021-01-07T06:37:17Z

/run-all-tests

fzhedu

LGTM

windtalker · 2021-01-07T06:53:55Z

/run-all-tests

windtalker · 2021-01-07T07:25:29Z

/run-unit-test

fzhedu · 2021-01-07T09:20:51Z

/merge

ti-srebot · 2021-01-07T09:20:54Z

Your auto merge job has been accepted, waiting for:

22197

ti-srebot · 2021-01-07T09:34:57Z

/run-all-tests

windtalker added 4 commits December 28, 2020 15:39

support explain analyze in mpp mode

715ab58

add task id info in explain statement for mpp

e3b786b

format output

e89a738

fix bug

42e027a

windtalker requested review from a team as code owners December 28, 2020 08:24

windtalker requested review from lzmhhh123 and removed request for a team December 28, 2020 08:24

windtalker changed the title ~~support explain analyze in mpp execution.~~ planner: support explain analyze in mpp execution. Dec 28, 2020

fix ci

7aa8aac

github-actions bot added the sig/execution SIG execution label Dec 28, 2020

windtalker changed the title ~~planner: support explain analyze in mpp execution.~~ execution: support explain analyze in mpp execution. Dec 28, 2020

windtalker added 5 commits December 28, 2020 17:47

refine code

744680a

fix ci

b1f2c5e

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

638a07f

…lain_info

refine

7cdf300

fix ci

5c65670

fzhedu reviewed Dec 29, 2020

View reviewed changes

util/execdetails/execdetails.go Outdated Show resolved Hide resolved

hanfei1991 reviewed Dec 29, 2020

View reviewed changes

ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 29, 2020

windtalker added 4 commits December 30, 2020 10:13

address comments

2a67d9e

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

4808348

…lain_info

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

5aebfea

…lain_info

merge master

2369d2c

fzhedu reviewed Jan 5, 2021

View reviewed changes

refine

22660c6

fzhedu reviewed Jan 7, 2021

View reviewed changes

fzhedu previously approved these changes Jan 7, 2021

View reviewed changes

ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 7, 2021

ti-srebot previously approved these changes Jan 7, 2021

View reviewed changes

merge master

d931ddf

windtalker dismissed stale reviews from ti-srebot and fzhedu via d931ddf January 7, 2021 05:15

ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 7, 2021

Merge branch 'master' into mpp_explain_info

7a81a99

fzhedu approved these changes Jan 7, 2021

View reviewed changes

Merge branch 'master' into mpp_explain_info

686fdb6

fzhedu and others added 2 commits January 7, 2021 17:21

Merge branch 'master' into mpp_explain_info

072d185

Merge branch 'master' into mpp_explain_info

e25764e

ti-srebot merged commit caa8c16 into pingcap:master Jan 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

execution: support explain analyze in mpp execution. #22053

execution: support explain analyze in mpp execution. #22053

windtalker commented Dec 28, 2020

sre-bot commented Dec 28, 2020

windtalker commented Dec 29, 2020

fzhedu Dec 29, 2020

hanfei1991 Dec 29, 2020

hanfei1991 left a comment

fzhedu commented Jan 5, 2021

fzhedu Jan 5, 2021

windtalker Jan 5, 2021

fzhedu Jan 6, 2021

hanfei1991 Jan 6, 2021

windtalker Jan 6, 2021

windtalker commented Jan 7, 2021

fzhedu Jan 7, 2021

windtalker Jan 7, 2021

fzhedu Jan 7, 2021

windtalker Jan 7, 2021

fzhedu Jan 7, 2021

windtalker Jan 7, 2021

fzhedu left a comment

fzhedu commented Jan 7, 2021

fzhedu commented Jan 7, 2021

ti-srebot commented Jan 7, 2021

ti-srebot commented Jan 7, 2021

ti-srebot commented Jan 7, 2021

windtalker commented Jan 7, 2021

windtalker commented Jan 7, 2021

fzhedu left a comment

windtalker commented Jan 7, 2021

windtalker commented Jan 7, 2021

fzhedu commented Jan 7, 2021

ti-srebot commented Jan 7, 2021

ti-srebot commented Jan 7, 2021

execution: support explain analyze in mpp execution. #22053

execution: support explain analyze in mpp execution. #22053

Conversation

windtalker commented Dec 28, 2020

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

sre-bot commented Dec 28, 2020

windtalker commented Dec 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanfei1991 left a comment

Choose a reason for hiding this comment

fzhedu commented Jan 5, 2021

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

windtalker commented Jan 7, 2021

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

fzhedu left a comment

Choose a reason for hiding this comment

fzhedu commented Jan 7, 2021

fzhedu commented Jan 7, 2021

ti-srebot commented Jan 7, 2021

ti-srebot commented Jan 7, 2021

ti-srebot commented Jan 7, 2021

windtalker commented Jan 7, 2021

windtalker commented Jan 7, 2021

fzhedu left a comment

Choose a reason for hiding this comment

windtalker commented Jan 7, 2021

windtalker commented Jan 7, 2021

fzhedu commented Jan 7, 2021

ti-srebot commented Jan 7, 2021

ti-srebot commented Jan 7, 2021