-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
executor: fix drop user bug #6624
Changes from 11 commits
f00bcea
a739674
f95fcfa
00123ea
7791b93
602a275
3f5ec4a
2054568
ba99296
89e5973
542dde5
9c62513
5cbbc29
1f05eef
ad10072
57f410e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -230,18 +230,34 @@ func (e *SimpleExec) executeDropUser(s *ast.DropUserStmt) error { | |
} | ||
continue | ||
} | ||
|
||
// begin a transaction to delete a user. | ||
if _, err := e.ctx.(sqlexec.SQLExecutor).Execute(context.Background(), "begin"); err != nil { | ||
return errors.Trace(err) | ||
} | ||
sql := fmt.Sprintf(`DELETE FROM %s.%s WHERE Host = "%s" and User = "%s";`, mysql.SystemDB, mysql.UserTable, user.Hostname, user.Username) | ||
_, _, err = e.ctx.(sqlexec.RestrictedSQLExecutor).ExecRestrictedSQL(e.ctx, sql) | ||
if err != nil { | ||
if _, err := e.ctx.(sqlexec.SQLExecutor).Execute(context.Background(), sql); err != nil { | ||
failedUsers = append(failedUsers, user.String()) | ||
} | ||
|
||
// delete privileges from mysql.db | ||
sql = fmt.Sprintf(`DELETE FROM %s.%s WHERE Host = "%s" and User = "%s";`, mysql.SystemDB, mysql.DBTable, user.Hostname, user.Username) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove this line? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about use a transaction to run the three delete statements? Or it will leave some grabege when meet something wrong. |
||
if _, err := e.ctx.(sqlexec.SQLExecutor).Execute(context.Background(), sql); err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
failedUsers = append(failedUsers, user.String()) | ||
} | ||
|
||
// delete privileges from mysql.tables_priv | ||
sql = fmt.Sprintf(`DELETE FROM %s.%s WHERE Host = "%s" and User = "%s";`, mysql.SystemDB, mysql.TablePrivTable, user.Hostname, user.Username) | ||
if _, err := e.ctx.(sqlexec.SQLExecutor).Execute(context.Background(), sql); err != nil { | ||
failedUsers = append(failedUsers, user.String()) | ||
} | ||
|
||
//TODO: need delete columns_priv once we implement columns_priv functionality. | ||
if _, err := e.ctx.(sqlexec.SQLExecutor).Execute(context.Background(), "commit"); err != nil { | ||
failedUsers = append(failedUsers, user.String()) | ||
} | ||
} | ||
if len(failedUsers) > 0 { | ||
// Commit the transaction even if we returns error | ||
err := e.ctx.Txn().Commit(sessionctx.SetCommitCtx(context.Background(), e.ctx)) | ||
if err != nil { | ||
return errors.Trace(err) | ||
} | ||
errMsg := "Operation DROP USER failed for " + strings.Join(failedUsers, ",") | ||
return terror.ClassExecutor.New(CodeCannotUser, errMsg) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to execute There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need. I just update accordingly. |
||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -177,6 +177,12 @@ func (s *testSuite) TestUser(c *C) { | |
tk.MustExec(createUserSQL) | ||
dropUserSQL = `DROP USER 'test1'@'localhost';` | ||
tk.MustExec(dropUserSQL) | ||
tk.MustQuery("select * from mysql.db").Check(testkit.Rows( | ||
"localhost test testDB Y Y Y Y Y Y Y N Y Y N N N N N N Y N N", | ||
"localhost test testDB1 Y Y Y Y Y Y Y N Y Y N N N N N N Y N N] [% dddb_% dduser Y Y Y Y Y Y Y N Y Y N N N N N N Y N N", | ||
"% test test Y N N N N N N N N N N N N N N N N N N", | ||
"localhost test testDBRevoke N N N N N N N N N N N N N N N N N N N", | ||
)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we need another test like dropping a user which not exists? |
||
} | ||
|
||
func (s *testSuite) TestSetPwd(c *C) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unlikely to meet a error here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? For example all the tikv nodes are down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before commit, all the operation is cached in union store @shenli
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete statement need to get data from tikv first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the err is not nil, we should return error.