Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

planner: fix wrong prepare plan after isolation read changed #16293

Merged

Conversation

lzmhhh123
Copy link
Contributor

@lzmhhh123 lzmhhh123 commented Apr 10, 2020

What problem does this PR solve?

Issue Number: close #16256

Problem Summary: After isolation read changed, the prepared statement should be re-prepared.

What is changed and how it works?

What's Changed: clear prepared plan when isolation read changes.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Performance regression
    • Consumes more CPU

Release note

  • Make plan cache consider isolation read engines.

@lzmhhh123 lzmhhh123 added type/bugfix This PR fixes a bug. sig/planner SIG: Planner needs-cherry-pick-4.0 labels Apr 10, 2020
@lzmhhh123 lzmhhh123 requested review from a team as code owners April 10, 2020 08:34
@ghost ghost requested review from SunRunAway and francis0407 and removed request for a team April 10, 2020 08:35
@lzmhhh123 lzmhhh123 requested review from eurekaka and winoros April 10, 2020 08:35
@github-actions github-actions bot added the sig/execution SIG execution label Apr 10, 2020
@@ -229,20 +230,24 @@ func (e *Execute) OptimizePreparedPlan(ctx context.Context, sctx sessionctx.Cont
}
}

if prepared.SchemaVersion != is.SchemaMetaVersion() {
if prepared.SchemaVersion != is.SchemaMetaVersion() || !reflect.DeepEqual(preparedObj.IsolationEngines, e.ctx.GetSessionVars().IsolationReadEngines) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about not use reflect.DeepEqual?

func prepare() (map[string]struct{}, map[string]struct{}) {
	m1 := make(map[string]struct{}, 3)
	m1["tikv"] = struct{}{}
	m1["tidb"] = struct{}{}
	m1["tiflash"] = struct{}{}
	m2 := make(map[string]struct{}, 3)
	m2["tikv"] = struct{}{}
	m2["tidb"] = struct{}{}
	m2["tiflash"] = struct{}{}
	return m1, m2
}

func Benchmark_cmp1(b *testing.B) {
	m1, m2 := prepare()
	for i := 0; i < b.N; i++ {
		reflect.DeepEqual(m1, m2)
	}
}

func Benchmark_cmp2(b *testing.B) {
	m1, m2 := prepare()
	for i := 0; i < b.N; i++ {
		checkEqualMap(m1, m2)
	}
}

func checkEqualMap(m1, m2 map[string]struct{}) bool {
	if len(m1) != len(m2) {
		return false
	}
	for k := range m1 {
		if _, ok := m2[k]; !ok {
			return false
		}
	}
	return true
}
go test -run '^$' -bench .
goos: darwin
goarch: amd64
pkg: github.com/crazycs520/test
Benchmark_cmp1-12        2272569               521 ns/op
Benchmark_cmp2-12       21689436                54.1 ns/op
PASS
ok      github.com/crazycs520/test      2.955s

@codecov
Copy link

codecov bot commented Apr 13, 2020

Codecov Report

Merging #16293 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #16293   +/-   ##
===========================================
  Coverage   79.8503%   79.8503%           
===========================================
  Files           520        520           
  Lines        139888     139888           
===========================================
  Hits         111701     111701           
  Misses        19231      19231           
  Partials       8956       8956           

@danmay319
Copy link
Contributor

LGTM

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 22, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Apr 22, 2020

/run-all-tests

@danmay319 danmay319 removed the status/can-merge Indicates a PR has been approved by a committer. label Apr 22, 2020
@danmay319 danmay319 requested a review from crazycs520 April 23, 2020 03:57
@zz-jason
Copy link
Member

zz-jason commented Apr 23, 2020

clear prepared plan when isolation read changes.

I prefer to add isolation read engines to the key of the prepared plan cache. It's different from schema change. schema version change is unpreventable, but isolation read engine change is. Once the user changed back to the original value of the isolation read engine, the cache can be used again at that time.

@eurekaka What's your opinion?

@eurekaka
Copy link
Contributor

clear prepared plan when isolation read changes.

I prefer to add isolation read engines to the key of the prepared plan cache. It's different from schema change. schema version change is unpreventable, but isolation read engine change is. Once the user changed back to the original value of the isolation read engine, the cache can be used again at that time.

@eurekaka What's your opinion?

+1

@lzmhhh123
Copy link
Contributor Author

clear prepared plan when isolation read changes.

I prefer to add isolation read engines to the key of the prepared plan cache. It's different from schema change. schema version change is unpreventable, but isolation read engine change is. Once the user changed back to the original value of the isolation read engine, the cache can be used again at that time.
@eurekaka What's your opinion?

+1

OK, I'll change it to that way.

@lzmhhh123 lzmhhh123 force-pushed the bug-fix/isolation_read_prepare_plan branch from 21a206b to bf54be0 Compare April 23, 2020 07:53
@lzmhhh123
Copy link
Contributor Author

/rebuild

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zz-jason zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label May 14, 2020
Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eurekaka eurekaka added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 1, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jun 1, 2020

Your auto merge job has been accepted, waiting for:

  • 17541

@sre-bot
Copy link
Contributor

sre-bot commented Jun 1, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 1, 2020

@lzmhhh123 merge failed.

@lzmhhh123
Copy link
Contributor Author

/merge

2 similar comments
@lzmhhh123
Copy link
Contributor Author

/merge

@lzmhhh123
Copy link
Contributor Author

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jun 1, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 1, 2020

@lzmhhh123 merge failed.

@sre-bot
Copy link
Contributor

sre-bot commented Jun 1, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 1, 2020

@lzmhhh123 merge failed.

@sre-bot
Copy link
Contributor

sre-bot commented Jun 1, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 1, 2020

@lzmhhh123 merge failed.

@lzmhhh123 lzmhhh123 merged commit 64f0d4d into pingcap:master Jun 1, 2020
@lzmhhh123 lzmhhh123 deleted the bug-fix/isolation_read_prepare_plan branch June 1, 2020 10:02
@lzmhhh123
Copy link
Contributor Author

/run-cherry-pick

@zz-jason
Copy link
Member

zz-jason commented Jun 1, 2020

/run-cherry-picker

@zz-jason zz-jason removed the request for review from francis0407 June 1, 2020 12:55
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Jun 1, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented Jun 1, 2020

cherry pick to release-4.0 in PR #17570

sre-bot added a commit that referenced this pull request Jun 6, 2020
…#17570)

* cherry pick #16293 to release-4.0

Signed-off-by: sre-bot <sre-bot@pingcap.com>

* fix fmt

Co-authored-by: Zhuomin(Charming) Liu <lzmhhh123@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution sig/planner SIG: Planner status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plan cache: the query plan doesn't change when tidb_isolation_read_engines be set to tifalsh
6 participants