Skip to content

*: check privilege when reusing the cached plan#12211

Merged
sre-bot merged 9 commits into
pingcap:masterfrom
cfzjywxk:cachePriv
Sep 23, 2019
Merged

*: check privilege when reusing the cached plan#12211
sre-bot merged 9 commits into
pingcap:masterfrom
cfzjywxk:cachePriv

Conversation

@cfzjywxk
Copy link
Copy Markdown
Contributor

@cfzjywxk cfzjywxk commented Sep 16, 2019

What problem does this PR solve?

Once prepared plan cache enabled and the cached plan hit need privilege check and table lock check

What is changed and how it works?

  • store visitinfo in newly defined PreparedObject(try not to add many things in ast structure)
  • check privilege when cached plan hit

pull requests #11970 cached execution is faced with same problem(see #11970 comments)

Check List

Tests

  • Unit test

Code changes

  • Has exported variable/fields change

Side effects

Related changes

Release note

@cfzjywxk cfzjywxk added type/bugfix This PR fixes a bug. sig/execution SIG execution labels Sep 16, 2019
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 16, 2019

Codecov Report

Merging #12211 into master will decrease coverage by 0.1527%.
The diff coverage is 66.1764%.

@@               Coverage Diff                @@
##             master     #12211        +/-   ##
================================================
- Coverage   81.2062%   81.0534%   -0.1528%     
================================================
  Files           454        454                
  Lines         99714      98947       -767     
================================================
- Hits          80974      80200       -774     
- Misses        12936      12951        +15     
+ Partials       5804       5796         -8

Comment thread session/session.go
if i > 0 {
plannercore.SetPstmtIDSchemaVersion(cacheKey, stmtID, s.sessionVars.PreparedStmts[stmtID].SchemaVersion)
if i > 0 && preparedAst != nil {
plannercore.SetPstmtIDSchemaVersion(cacheKey, stmtID, preparedAst.SchemaVersion)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe s.sessionVars.PreparedStmts[stmtID].SchemaVersion != preparedAst.SchemaVersion and this will let s.PreparedPlanCache().Delete(cacheKey) leave some sth that need be delete

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is a problem. Current prepared plan cache logic does not handle this and rely on lru list length limit to remove unused plan object, maybe we need to do removal in getPhysicalPlan when schema version changed

systems map[string]string
// PreparedStmts stores prepared statement.
PreparedStmts map[uint32]*ast.Prepared
PreparedStmts map[uint32]interface{}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we use map[uint32]*plannercore.PrepareObject directly for this field?

systems map[string]string
// PreparedStmts stores prepared statement.
PreparedStmts map[uint32]*ast.Prepared
PreparedStmts map[uint32]interface{}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about directly store PrepareObject?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've tried this. PrepareObject stores visitInfo(this in planner ) field, seems SessionVars struct in variable package cannot import planner related package, there would be cycle
@lysu @zz-jason

Comment thread planner/core/planbuilder.go Outdated
@zz-jason zz-jason changed the title executor: add priv check for cached plan execution executor: check privilege when reusing the cached execution plan Sep 16, 2019
@zz-jason zz-jason changed the title executor: check privilege when reusing the cached execution plan executor: check privilege when reusing the cached plan Sep 16, 2019
@cfzjywxk cfzjywxk requested review from lysu and zz-jason September 16, 2019 13:28
Copy link
Copy Markdown
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.

please merge master and resolve conflicts.

Comment thread server/conn_stmt.go Outdated
@cfzjywxk cfzjywxk requested a review from zz-jason September 17, 2019 10:01
Copy link
Copy Markdown
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 Sep 17, 2019
@zz-jason zz-jason changed the title executor: check privilege when reusing the cached plan *: check privilege when reusing the cached plan Sep 17, 2019
@cfzjywxk
Copy link
Copy Markdown
Contributor Author

@jackysp @lysu PTAL thx~

jackysp
jackysp previously approved these changes Sep 23, 2019
Copy link
Copy Markdown
Contributor

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp jackysp added the status/can-merge Indicates a PR has been approved by a committer. label Sep 23, 2019
@jackysp
Copy link
Copy Markdown
Contributor

jackysp commented Sep 23, 2019

Please resolve the conflict, @cfzjywxk .

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 23, 2019
@zz-jason
Copy link
Copy Markdown
Member

/merge

@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Sep 23, 2019

/run-all-tests

@sre-bot sre-bot merged commit 06629d6 into pingcap:master Sep 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sig/execution SIG execution 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.

5 participants