planner: support create binding from history #39436

fzzf678 · 2022-11-28T14:41:15Z

What problem does this PR solve?

Issue Number: ref #39199

Problem Summary:

What is changed and how it works?

Support create binding from history using plan digest

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

Please refer to Release Notes Language Style Guide to write a quality release note.

None

ti-chi-bot · 2022-11-28T14:41:18Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Reminiscent
qw4990

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.

… binding_create

qw4990 · 2022-11-29T15:11:34Z

planner/cascades/testdata/integration_suite_out.json

@@ -1098,7 +1098,7 @@
          "Projection_3 10000.00 root  Column#2, Column#3, Column#4, Column#5, Column#6, Column#7, Column#8, Column#9",
          "└─MemTableScan_4 10000.00 root table:PROCESSLIST "
        ],
-        "Result": ["  test Sleep 0 autocommit select USER, HOST, DB, COMMAND, TIME, STATE, INFO, DIGEST from information_schema.processlist 7284ef6cf62f14bccb9fa24aff861b419e9cf59b2c098d3069c7c10bc35d0c14"]
+        "Result": ["  test Sleep 0 autocommit select USER, HOST, DB, COMMAND, TIME, STATE, INFO, DIGEST from information_schema.processlist 786234ab0b57f68ad9452021ba5299ab0c1e879a37c8810ceeaf4a1074fe9daa"]


Why is it changed?

I think it's because we add new key words DIGEST and the parser changed

qw4990 · 2022-11-29T15:12:07Z

planner/core/common_plans.go

@@ -265,6 +265,7 @@ type SQLBindPlan struct {
 	Charset      string
 	Collation    string
 	NewStatus    string
+	Source       string


Please add some comments for this new field.

qw4990 · 2022-11-29T15:12:52Z

planner/core/preprocess.go

-		EraseLastSemicolon(node.OriginNode)
-		EraseLastSemicolon(node.HintedNode)
-		p.checkBindGrammar(node.OriginNode, node.HintedNode, p.sctx.GetSessionVars().CurrentDB)
+		if node.PlanDigest == "" {


Please add some comments like 'if planDigest is not empty, then ...';

qw4990 · 2022-11-29T15:16:57Z

planner/core/planbuilder.go

+		logutil.BgLogger().Debug("[sql-bind] parse origin SQL failed in create binding from history",
+			zap.String("SQL", bindableStmt.Query), zap.Error(err))


Should we return this error directly here?

qw4990 · 2022-11-29T15:17:50Z

planner/core/planbuilder.go

+		logutil.BgLogger().Debug("[sql-bind] parse origin SQL failed in create binding from history",
+			zap.String("SQL", bindableStmt.Query), zap.Error(err))
+	}
+	bindSQL := bindinfo.GenerateBindSQL(context.TODO(), originNode, bindableStmt.PlanHint, true, bindableStmt.Schema)


The captured argument should be false.

Reminiscent · 2022-12-01T08:20:46Z

bindinfo/bind_test.go

@@ -1388,3 +1388,50 @@ func TestDropBindBySQLDigest(t *testing.T) {
 	tk.MustGetErrMsg(fmt.Sprintf("drop binding for sql digest '%s'", "1"), "can't find any binding for `1`")
 	tk.MustGetErrMsg(fmt.Sprintf("drop binding for sql digest '%s'", ""), "sql digest is empty")
 }
+
+func TestCreateBindingFromHistory(t *testing.T) {


If the binding already exists, what will happen after create binding from history?

will be covered, only the newest exists

Reminiscent · 2022-12-01T08:21:17Z

planner/core/planbuilder.go

@@ -1039,7 +1040,48 @@ func checkHintedSQL(sql, charset, collation, db string) error {
 	return nil
 }

+func (b *PlanBuilder) buildCreateBindPlanFromPlanDigest(v *ast.CreateBindingStmt) (Plan, error) {
+	if v.PlanDigest == "" {
+		return nil, errors.New("sql digest is empty")


plan digest is empty?

planner/core/planbuilder.go

Co-authored-by: Chengpeng Yan <41809508+Reminiscent@users.noreply.github.com>

qw4990 · 2022-12-01T09:31:39Z

/merge

ti-chi-bot · 2022-12-01T09:31:43Z

This pull request has been accepted and is ready to merge.

Commit hash: bbfb53d

This reverts commit bac93cf.

fzzf678 · 2022-12-01T11:21:04Z

/merge

ti-chi-bot · 2022-12-01T11:21:08Z

This pull request has been accepted and is ready to merge.

Commit hash: 3c3bf82

sre-bot · 2022-12-01T12:59:12Z

TiDB MergeCI notify

🔴 Bad News! [2] CI still failing after this pr merged.
These failed integration tests don't seem to be introduced by the current PR.

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/integration-common-test	🔴 failed 2, success 15, total 17	1 hr 3 min	Existing failure
idc-jenkins-ci/integration-cdc-test	🔴 failed 1, success 39, total 40	25 min	Existing failure
idc-jenkins-ci-tidb/common-test	🟢 all 11 tests passed	10 min	Existing passed
idc-jenkins-ci-tidb/integration-ddl-test	🟢 all 6 tests passed	8 min 4 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	6 min 41 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	6 min 31 sec	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	6 min 24 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	4 min 1 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	2 min 39 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

fzzf678 added 2 commits November 15, 2022 22:21

parser

677d60a

create from local statements summary

e97ca7d

ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Nov 28, 2022

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 28, 2022

Merge remote-tracking branch 'upstream/master' into binding_create

cca07f8

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 28, 2022

fzzf678 added 2 commits November 28, 2022 22:46

Merge branch 'master' into binding_create

068bf96

parser

9a35816

ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 28, 2022

sort import

2d0c3ec

fzzf678 added epic/sql-plan-management sig/planner SIG: Planner labels Nov 28, 2022

fzzf678 added 7 commits November 28, 2022 23:08

Update planbuilder.go

f4ebb90

Merge branch 'master' into binding_create

53c768b

digest change

d8fcf60

Merge branch 'binding_create' of https://github.com/fzzf678/tidb into…

883381f

… binding_create

create source

f924575

Merge branch 'master' into binding_create

7abb532

Merge branch 'master' into binding_create

bd94d25

qw4990 reviewed Nov 29, 2022

View reviewed changes

fzzf678 added 2 commits December 1, 2022 12:14

Merge remote-tracking branch 'upstream/master' into binding_create

2187222

Update parser.go

2903c7a

Reminiscent reviewed Dec 1, 2022

View reviewed changes

planner/core/planbuilder.go Outdated Show resolved Hide resolved

Update planner/core/planbuilder.go

85979e8

Co-authored-by: Chengpeng Yan <41809508+Reminiscent@users.noreply.github.com>

Reminiscent approved these changes Dec 1, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 1, 2022

fzzf678 added 3 commits December 1, 2022 16:48

Update planbuilder.go

693c88f

supply ut and the symbol

8ac29ff

comment

17e6791

qw4990 approved these changes Dec 1, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 1, 2022

Merge branch 'master' into binding_create

bbfb53d

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 1, 2022

bazel

bac93cf

fzzf678 requested a review from a team as a code owner December 1, 2022 09:52

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Dec 1, 2022

fzzf678 added 3 commits December 1, 2022 18:02

Revert "bazel"

0c31e8a

This reverts commit bac93cf.

Update BUILD.bazel

d216bd9

Merge branch 'master' into binding_create

3c3bf82

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 1, 2022

Merge branch 'master' into binding_create

4f622fb

ti-chi-bot merged commit 202f723 into pingcap:master Dec 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: support create binding from history #39436

planner: support create binding from history #39436

fzzf678 commented Nov 28, 2022

ti-chi-bot commented Nov 28, 2022 •

edited

Loading

qw4990 Nov 29, 2022

fzzf678 Nov 30, 2022

qw4990 Nov 29, 2022

qw4990 Nov 29, 2022

qw4990 Nov 29, 2022

qw4990 Nov 29, 2022

Reminiscent Dec 1, 2022

fzzf678 Dec 1, 2022

Reminiscent Dec 1, 2022

qw4990 commented Dec 1, 2022

ti-chi-bot commented Dec 1, 2022

fzzf678 commented Dec 1, 2022

ti-chi-bot commented Dec 1, 2022

sre-bot commented Dec 1, 2022

		logutil.BgLogger().Debug("[sql-bind] parse origin SQL failed in create binding from history",
		zap.String("SQL", bindableStmt.Query), zap.Error(err))

planner: support create binding from history #39436

planner: support create binding from history #39436

Conversation

fzzf678 commented Nov 28, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Nov 28, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qw4990 commented Dec 1, 2022

ti-chi-bot commented Dec 1, 2022

fzzf678 commented Dec 1, 2022

ti-chi-bot commented Dec 1, 2022

sre-bot commented Dec 1, 2022

TiDB MergeCI notify

ti-chi-bot commented Nov 28, 2022 •

edited

Loading