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 update privilege error #10085

Merged
merged 9 commits into from
May 7, 2019

Conversation

haplone
Copy link
Contributor

@haplone haplone commented Apr 9, 2019

What problem does this PR solve?

issue10028

update ap.record t inner join tp.record tt on t.id=tt.id  set t.name=tt.name;
ERROR 1105 (HY000): privilege check fail

What is changed and how it works?

get the right database name for update privilege check in planner/core/logical_plan_builder.go#buildUpdate

Check List

Tests

  • Unit test

Code changes

Side effects

Related changes

we need to fix in release-2.0 release-2.1 by another pr(buildUpdate logic has changed)

Copy link
Contributor

@xiekeyi98 xiekeyi98 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.

@@ -2611,6 +2614,13 @@ func (b *PlanBuilder) buildUpdateLists(tableList []*ast.TableName, list []*ast.A
col := assign.Col

dbName := col.DBName.L
// With sql below we need to get database by table alia name
Copy link
Contributor

Choose a reason for hiding this comment

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

What the alia stand for?

Suggested change
// With sql below we need to get database by table alia name
// With sql below, we need to get database by the table alias name.

@@ -2611,6 +2614,13 @@ func (b *PlanBuilder) buildUpdateLists(tableList []*ast.TableName, list []*ast.A
col := assign.Col

dbName := col.DBName.L
// With sql below we need to get database by table alia name
// update ap.record t inner join tp.record tt on t.id=tt.id set t.name=tt.name
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// update ap.record t inner join tp.record tt on t.id=tt.id set t.name=tt.name
// issue#10028

@xiekeyi98 xiekeyi98 changed the title fix update privilege error #10028 planner: fix update privilege error Apr 9, 2019
@xiekeyi98 xiekeyi98 added sig/planner SIG: Planner contribution This PR is from a community contributor. labels Apr 9, 2019
@codecov
Copy link

codecov bot commented Apr 10, 2019

Codecov Report

Merging #10085 into master will decrease coverage by 0.0028%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master     #10085        +/-   ##
================================================
- Coverage   77.3341%   77.3312%   -0.0029%     
================================================
  Files           412        412                
  Lines         85622      85629         +7     
================================================
+ Hits          66215      66218         +3     
- Misses        14389      14390         +1     
- Partials       5018       5021         +3

Copy link

@imtbkcat imtbkcat left a comment

Choose a reason for hiding this comment

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

LGTM

@imtbkcat imtbkcat added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 15, 2019
@imtbkcat
Copy link

/run-all-tests

@@ -2611,6 +2614,12 @@ func (b *PlanBuilder) buildUpdateLists(tableList []*ast.TableName, list []*ast.A
col := assign.Col

dbName := col.DBName.L
// To solve issue#10028, we need to get database name by the table alias name.
for _, tbl := range tableList {
Copy link
Member

Choose a reason for hiding this comment

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

will it be faster if we pre-build a map[tblName]DBName and save this for loop?

// To solve issue#10028, we need to get database name by the table alias name.
for _, tbl := range tableList {
if strings.EqualFold(tbl.Name.L, col.TblName.L) {
dbName = tbl.DBInfo.Name.L
Copy link
Member

Choose a reason for hiding this comment

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

BTW, after finding a dbName, we should break.

@haplone
Copy link
Contributor Author

haplone commented May 6, 2019

@zz-jason
PTAL

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 status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 7, 2019
@zz-jason
Copy link
Member

zz-jason commented May 7, 2019

/run-all-tests

@zz-jason
Copy link
Member

zz-jason commented May 7, 2019

/run-mybatis-test

@zz-jason zz-jason merged commit 041e0e9 into pingcap:master May 7, 2019
@zz-jason
Copy link
Member

zz-jason commented May 7, 2019

@haplone Please cherry pick this commit to the release-2.1 branch

@haplone
Copy link
Contributor Author

haplone commented May 8, 2019

@zz-jason
I found branch release 2.1 on tidb has an error,
the mysql.UpdatePriv should be mysql.SelectPriv
if it is fixed ,we could solve this issue in release 2.1 by cherry pick , or I have to make a new pr:
https://github.com/pingcap/tidb/blob/release-2.1/planner/core/logical_plan_builder.go#L2153

func (b *planBuilder) buildUpdate(update *ast.UpdateStmt) (Plan, error) {
       // ...
        for _, t := range tableList {
		dbName := t.Schema.L
		if dbName == "" {
			dbName = b.ctx.GetSessionVars().CurrentDB
		}
		// the mysql.UpdatePriv should be mysql.SelectPriv
		b.visitInfo = appendVisitInfo(b.visitInfo, mysql.UpdatePriv, dbName, t.Name.L, "")
	}

@zz-jason
Copy link
Member

seems we need to cherry pick #8376 to release 2.1 first @tiancaiamao

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/privilege contribution This PR is from a community contributor. sig/planner SIG: Planner status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants