*: Modify tidb/executor to implement plan recreator #26381

zhuo-zhi · 2021-07-20T09:33:50Z

What problem does this PR solve?

Issue Number: close #26335, subitem of #26325

Problem Summary:

Subtask of Plan Recreator.
Modify tidb/executor to implement plan recreator

What is changed and how it works?

Modify executor to support plan recreator's dumping files of a single SQL statement.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Modify tidb/executor to implement plan recreator

ti-chi-bot · 2021-07-20T09:33:51Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

ti-srebot · 2021-07-20T09:34:29Z

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

ti-srebot · 2021-07-20T09:48:50Z

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

ti-srebot · 2021-07-21T08:44:39Z

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

zhuo-zhi · 2021-07-21T09:32:49Z

Blocked by parser pull request.

rebelice · 2021-07-28T02:57:37Z

executor/plan_recreator.go

+
+	// Dump bindings
+	normSql, _ := parser.NormalizeDigest(e.ExecStmt.Text())
+	recordSets, err = e.Ctx.(sqlexec.SQLExecutor).Execute(context.TODO(), fmt.Sprintf("show bindings where Original_sql='%s'", normSql))


I think we‘d better dump all bindings.

Done in the new commit.

…creator

ti-chi-bot · 2021-07-28T08:01:13Z

@zhuo-zhi: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

zhuo-zhi · 2021-07-28T08:03:25Z

Moved some code to pr #26700

time-and-fate · 2021-07-30T11:39:06Z

executor/plan_recreator.go

+	return sRows, nil
+}
+
+func getRows4Test(ctx context.Context, rs sqlexec.RecordSet) ([]chunk.Row, error) {


Can we use DrainRecordSet() directly? I think they're similar.

It's only used for oom action.

executor/plan_recreator.go

Co-authored-by: Zhou Kunqin <[email protected]>

qw4990 · 2021-08-09T10:04:10Z

executor/plan_recreator.go

+	}
+	return in, true
+}
+
 // planRecreatorVarKeyType is a dummy type to avoid naming collision in context.
 type planRecreatorVarKeyType int


Can this variable be removed?

qw4990 · 2021-08-09T10:06:25Z

executor/plan_recreator.go

+	result := newFirstChunk(e)
+	result.AppendString(0, res)
+	req.Append(result, 0, 1)


We can directly use req.AppendString(0, res) here.

qw4990 · 2021-08-09T10:10:09Z

executor/plan_recreator.go

 	}

 	// Create zip file
 	startTime := time.Now()
 	fileName := fmt.Sprintf("recreator_single_%v.zip", startTime.UnixNano())
-	zf, err := os.Create(recreatorPath + "/" + fileName)
+	zf, err := os.Create(path + "/" + fileName)


Please use filepath.Join(path, fileName) here, which is safer than path + "/" + fileName.

qw4990 · 2021-08-09T10:10:29Z

executor/plan_recreator.go

@@ -145,7 +177,7 @@ func (e *PlanRecreatorSingleInfo) dumpSingle() (string, error) {
 		TList := val.(fileList).TokenMap
 		for k, v := range Flist {
 			if time.Since(v.StartTime).Minutes() > remainedInterval {
-				err := os.Remove(recreatorPath + "/" + k)
+				err := os.Remove(path + "/" + k)


qw4990

Please add some test cases for this feature, you can refer to select ... into outfile TestSelectIntoOutfileFromTable.

qw4990 · 2021-08-09T10:20:19Z

planner/core/logical_plans.go

+	ExecStmt ast.StmtNode
+	Analyze  bool
+	Load     bool
+	File     string


These fields seem not be used?

time-and-fate · 2021-08-10T03:54:43Z

session/session.go

+	if s.Value(executor.PlanRecreatorFileList) != nil {
+		executor.CleanUpPlanRecreatorFile(s.GetSessionVars().ConnectionID)
+	}


Does this mean if the client closed this session, the file would be no longer available? Is this expected?

time-and-fate · 2021-08-10T05:25:34Z

executor/plan_recreator.go

+	return nil
+}
+
+func resultSetToStringSlice(ctx context.Context, rs sqlexec.RecordSet) ([][]string, error) {


Seems we can directly use ResultSetToStringSlice from the session package?

time-and-fate · 2021-08-10T05:26:40Z

Also please fix the lint error.

ti-chi-bot · 2021-08-22T08:08:14Z

@zhuo-zhi: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Modify tidb/executor to implement plan recreator

1de46ff

ti-chi-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 20, 2021

zhuo-zhi marked this pull request as draft July 20, 2021 09:34

ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 20, 2021

github-actions bot added sig/execution SIG execution sig/sql-infra SIG: SQL Infra labels Jul 20, 2021

Refine some comments

99fd2d7

support dumping all files related to single sql

b2b636e

add plan recreator unit test

c1bc141

zhuo-zhi added 2 commits July 22, 2021 18:18

Support cleaning outdated files

ef88000

Support generating token

ffb33ae

rebelice reviewed Jul 28, 2021

View reviewed changes

zhuo-zhi added 6 commits July 28, 2021 15:01

update parser to version 20210728060616

6f72dc5

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

420ae6c

…creator

Merge branch 'o_master' into plan_recreator

903e99d

go mod tidy

6b8ade2

Merge branch 'o_master' into plan_recreator

009f00c

Merge branch 'master' into plan_recreator

8198f13

zhuo-zhi mentioned this pull request Jul 28, 2021

*: Add plan recreator executor framework #26700

Merged

12 tasks

zhuo-zhi marked this pull request as ready for review July 28, 2021 07:58

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 28, 2021

ti-chi-bot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Jul 28, 2021

make fmt

a6b99b7

zhuo-zhi added 3 commits July 28, 2021 17:02

reformat code

e9dbe3b

reformat code

97a927d

add PlanRecreatorInfo interface

6e0c56c

ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 30, 2021

time-and-fate reviewed Jul 30, 2021

View reviewed changes

Merge branch 'master' into plan_recreator

aebf86c

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 30, 2021

time-and-fate reviewed Jul 30, 2021

View reviewed changes

executor/plan_recreator.go Outdated Show resolved Hide resolved

executor/plan_recreator.go Show resolved Hide resolved

zhouqiang-cl removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Aug 4, 2021

Update executor/plan_recreator.go

109fe2e

Co-authored-by: Zhou Kunqin <[email protected]>

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 4, 2021

Merge branch 'master' into plan_recreator

b49c4b4

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 4, 2021

rebelice and others added 2 commits August 5, 2021 04:00

clean up files when closing session

986fe56

Merge branch 'master' into plan_recreator

0d04697

qw4990 self-requested a review August 5, 2021 06:59

rebelice added 2 commits August 9, 2021 17:43

remove plan recreator from server/conn.go

b99803f

Merge remote-tracking branch 'zhuo-zhi/plan_recreator' into HEAD

186c6e6

qw4990 reviewed Aug 9, 2021

View reviewed changes

time-and-fate reviewed Aug 10, 2021

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: Modify tidb/executor to implement plan recreator #26381

*: Modify tidb/executor to implement plan recreator #26381

zhuo-zhi commented Jul 20, 2021 •

edited

Loading

ti-chi-bot commented Jul 20, 2021

ti-srebot commented Jul 20, 2021

ti-srebot commented Jul 20, 2021

ti-srebot commented Jul 21, 2021

zhuo-zhi commented Jul 21, 2021

rebelice Jul 28, 2021

zhuo-zhi Jul 28, 2021

ti-chi-bot commented Jul 28, 2021

zhuo-zhi commented Jul 28, 2021

time-and-fate Jul 30, 2021 •

edited

Loading

rebelice Aug 5, 2021

qw4990 Aug 9, 2021

qw4990 Aug 9, 2021

qw4990 Aug 9, 2021

qw4990 Aug 9, 2021

qw4990 left a comment

qw4990 Aug 9, 2021

time-and-fate Aug 10, 2021

time-and-fate Aug 10, 2021

time-and-fate commented Aug 10, 2021

ti-chi-bot commented Aug 22, 2021

*: Modify tidb/executor to implement plan recreator #26381

Are you sure you want to change the base?

*: Modify tidb/executor to implement plan recreator #26381

Conversation

zhuo-zhi commented Jul 20, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Jul 20, 2021

ti-srebot commented Jul 20, 2021

ti-srebot commented Jul 20, 2021

ti-srebot commented Jul 21, 2021

zhuo-zhi commented Jul 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot commented Jul 28, 2021

zhuo-zhi commented Jul 28, 2021

time-and-fate Jul 30, 2021 • 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

qw4990 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

time-and-fate commented Aug 10, 2021

ti-chi-bot commented Aug 22, 2021

zhuo-zhi commented Jul 20, 2021 •

edited

Loading

time-and-fate Jul 30, 2021 •

edited

Loading