executor: fix drop user bug #6624

zhexuany · 2018-05-23T07:52:00Z


mysql> create database tidb;
Query OK, 0 rows affected (0.01 sec)

mysql> 
mysql> select * from mysql.db;
Empty set (0.00 sec)

mysql> GRANT ALL PRIVILEGES ON tidb.* TO 'tidb'@'%' WITH GRANT OPTION;
Query OK, 1 row affected (0.01 sec)

mysql> flush privileges;
Query OK, 0 rows affected (0.00 sec)

mysql> drop user 'tidb'@'%';
Query OK, 0 rows affected (0.01 sec)

mysql> 
mysql> select * from mysql.db\G;
*************************** 1. row ***************************
                 Host: %
                   DB: tidb
                 User: tidb
          Select_priv: Y
          Insert_priv: Y
          Update_priv: Y
          Delete_priv: Y
          Create_priv: Y
            Drop_priv: Y
           Grant_priv: Y
      References_priv: N
           Index_priv: Y
           Alter_priv: Y
Create_tmp_table_priv: N
     Lock_tables_priv: N
     Create_view_priv: N
       Show_view_priv: N
  Create_routine_priv: N
   Alter_routine_priv: N
         Execute_priv: Y
           Event_priv: N
         Trigger_priv: N
1 row in set (0.00 sec)


mysql> flush privileges;
Query OK, 0 rows affected (0.01 sec)

mysql> select * from mysql.db\G;
*************************** 1. row ***************************
                 Host: %
                   DB: tidb
                 User: tidb
          Select_priv: Y
          Insert_priv: Y
          Update_priv: Y
          Delete_priv: Y
          Create_priv: Y
            Drop_priv: Y
           Grant_priv: Y
      References_priv: N
           Index_priv: Y
           Alter_priv: Y
Create_tmp_table_priv: N
     Lock_tables_priv: N
     Create_view_priv: N
       Show_view_priv: N
  Create_routine_priv: N
   Alter_routine_priv: N
         Execute_priv: Y
           Event_priv: N
         Trigger_priv: N
1 row in set (0.00 sec)

MySQL 5.7

{code:sql}
MySQL [(none)]> show schemas;
+--------------------+
| Database           |
+--------------------+
| information_schema |
| mysql              |
| performance_schema |
| sys                |
| test               |
+--------------------+
5 rows in set (0.00 sec)


MySQL [(none)]> create database tidb;
Query OK, 1 row affected (0.00 sec)

MySQL [(none)]> CREATE USER 'tidb'@'%' IDENTIFIED BY 'password';
Query OK, 0 rows affected (0.00 sec)

MySQL [(none)]> GRANT ALL PRIVILEGES ON tidb.* TO 'tidb'@'%' WITH GRANT OPTION;
Query OK, 0 rows affected (0.00 sec)

MySQL [(none)]> flush privileges;
Query OK, 0 rows affected (0.00 sec)

mysql> select * from mysql.db\G;
*************************** 1. row ***************************
                 Host: localhost
                   Db: performance_schema
                 User: mysql.session
          Select_priv: Y
          Insert_priv: N
          Update_priv: N
          Delete_priv: N
          Create_priv: N
            Drop_priv: N
           Grant_priv: N
      References_priv: N
           Index_priv: N
           Alter_priv: N
Create_tmp_table_priv: N
     Lock_tables_priv: N
     Create_view_priv: N
       Show_view_priv: N
  Create_routine_priv: N
   Alter_routine_priv: N
         Execute_priv: N
           Event_priv: N
         Trigger_priv: N
*************************** 2. row ***************************
                 Host: localhost
                   Db: sys
                 User: mysql.sys
          Select_priv: N
          Insert_priv: N
          Update_priv: N
          Delete_priv: N
          Create_priv: N
            Drop_priv: N
           Grant_priv: N
      References_priv: N
           Index_priv: N
           Alter_priv: N
Create_tmp_table_priv: N
     Lock_tables_priv: N
     Create_view_priv: N
       Show_view_priv: N
  Create_routine_priv: N
   Alter_routine_priv: N
         Execute_priv: N
           Event_priv: N
         Trigger_priv: Y
*************************** 3. row ***************************
                 Host: %
                   Db: tidb
                 User: tidb
          Select_priv: Y
          Insert_priv: Y
          Update_priv: Y
          Delete_priv: Y
          Create_priv: Y
            Drop_priv: Y
           Grant_priv: Y
      References_priv: Y
           Index_priv: Y
           Alter_priv: Y
Create_tmp_table_priv: Y
     Lock_tables_priv: Y
     Create_view_priv: Y
       Show_view_priv: Y
  Create_routine_priv: Y
   Alter_routine_priv: Y
         Execute_priv: Y
           Event_priv: Y
         Trigger_priv: Y
3 rows in set (0.00 sec)

MySQL [(none)]> drop user 'tidb'@'%';
Query OK, 0 rows affected (0.00 sec)

mysql> select * from mysql.db\G;
*************************** 1. row ***************************
                 Host: localhost
                   Db: performance_schema
                 User: mysql.session
          Select_priv: Y
          Insert_priv: N
          Update_priv: N
          Delete_priv: N
          Create_priv: N
            Drop_priv: N
           Grant_priv: N
      References_priv: N
           Index_priv: N
           Alter_priv: N
Create_tmp_table_priv: N
     Lock_tables_priv: N
     Create_view_priv: N
       Show_view_priv: N
  Create_routine_priv: N
   Alter_routine_priv: N
         Execute_priv: N
           Event_priv: N
         Trigger_priv: N
*************************** 2. row ***************************
                 Host: localhost
                   Db: sys
                 User: mysql.sys
          Select_priv: N
          Insert_priv: N
          Update_priv: N
          Delete_priv: N
          Create_priv: N
            Drop_priv: N
           Grant_priv: N
      References_priv: N
           Index_priv: N
           Alter_priv: N
Create_tmp_table_priv: N
     Lock_tables_priv: N
     Create_view_priv: N
       Show_view_priv: N
  Create_routine_priv: N
   Alter_routine_priv: N
         Execute_priv: N
           Event_priv: N
         Trigger_priv: Y
2 rows in set (0.00 sec)

zhexuany · 2018-05-23T07:55:40Z

After drop user, mysql.db does not have any row related with user which was intended to drop but tidb has such row. This PR fixes this.

shenli · 2018-05-23T07:56:33Z

executor/simple.go

+		} else {
+			sql = fmt.Sprintf(`DELETE FROM %s.%s WHERE Host = "%s" and User = "%s";`, mysql.SystemDB, mysql.DBTable, user.Hostname, user.Username)
+		}
+		sql = fmt.Sprintf(`DELETE FROM %s.%s WHERE Host = "%s" and User = "%s";`, mysql.SystemDB, mysql.DBTable, user.Hostname, user.Username)


Remove this line?

alivxxx · 2018-05-23T09:07:37Z

executor/simple.go

@@ -232,6 +232,13 @@ func (e *SimpleExec) executeDropUser(s *ast.DropUserStmt) error {
 		}
 		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)


This err will be overwritten.

winkyao

Please address comment, reset LGTM

zhexuany · 2018-05-23T11:20:52Z

/run-all-tests

shenli · 2018-05-23T14:29:16Z

@zhexuany We need to clean up data in the tables_priv and columns_priv.

/cc @tiancaiamao

zhexuany · 2018-05-24T05:57:28Z

@tiancaiamao PTAL

shenli · 2018-05-24T06:38:46Z

executor/simple.go

@@ -235,6 +235,22 @@ func (e *SimpleExec) executeDropUser(s *ast.DropUserStmt) error {
 		if 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)


How about use a transaction to run the three delete statements? Or it will leave some grabege when meet something wrong.

tiancaiamao

Test MySQL's behavior:

begin
insert into t value (1)
drop user tidb
rollback
select * from t

tiancaiamao · 2018-05-24T11:41:27Z

executor/simple.go

-		_, _, 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())


It's unlikely to meet a error here.

Why? For example all the tikv nodes are down.

Before commit, all the operation is cached in union store @shenli

Delete statement need to get data from tikv first.

If the err is not nil, we should return error.

tiancaiamao · 2018-05-24T11:42:04Z

executor/simple.go

+
+		//TODO: (tidb-team) need delete columns_priv once we implement columns_priv functionality.
+		if _, err := e.ctx.(sqlexec.SQLExecutor).Execute(context.Background(), "commit"); err != nil {
+			return errors.Trace(err)


failedUsers = append(failedUsers, user.String())

tiancaiamao · 2018-05-24T11:42:54Z

executor/simple.go

-		if err != nil {
-			return errors.Trace(err)
-		}
+		// err := e.ctx.Txn().Commit(sessionctx.SetCommitCtx(context.Background(), e.ctx))


Clean up the code

zhexuany · 2018-05-25T08:24:30Z

@shenli @tiancaiamao PTAL

tiancaiamao · 2018-05-25T09:10:44Z

executor/simple.go

+
+		// begin a transaction to delete a user.
+		if _, err := e.ctx.(sqlexec.SQLExecutor).Execute(context.Background(), "begin"); err != nil {
+			fmt.Println(err)


remove debug log

tiancaiamao · 2018-05-25T09:11:57Z

executor/simple.go

-		_, _, 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())


Before commit, all the operation is cached in union store @shenli

tiancaiamao · 2018-05-25T09:14:27Z

Will MySQL drop user tidb auto commit current transaction? what's its behavior? @zhexuany

zhexuany · 2018-05-30T01:32:30Z

I will be auto committed. @tiancaiamao

zhexuany · 2018-05-30T01:32:43Z

@shenli @tiancaiamao PTAL again.

tiancaiamao · 2018-05-30T05:36:53Z

LGTM

shenli · 2018-05-30T09:06:59Z

executor/simple.go

+
+		// 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)
+		if _, err := e.ctx.(sqlexec.SQLExecutor).Execute(context.Background(), sql); err != nil {


zhexuany · 2018-06-04T03:58:24Z

@shenli PTAL

zz-jason · 2018-06-04T04:34:26Z

executor/simple.go

+			failedUsers = append(failedUsers, user.String())
+		}
+
+		//TODO: (tidb-team) need delete columns_priv once we implement columns_priv functionality.


no need to add "(tidb-team)" in todo

zhexuany · 2018-06-04T07:05:16Z

@zz-jason PTAL again.

zz-jason · 2018-06-05T00:49:19Z

executor/simple.go

-		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)


Do we need to execute rollback when len(failedUsers) > 0?

We need. I just update accordingly.

zz-jason · 2018-06-05T00:51:07Z

executor/simple_test.go

+		"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",
+	))


Maybe we need another test like dropping a user which not exists?

shenli · 2018-06-06T07:25:46Z

LGTM

zhexuany · 2018-06-06T08:04:14Z

/run-all-test

zz-jason · 2018-06-06T08:09:00Z

executor/simple_test.go

+
+	createUserSQL = `CREATE USER 'test1'@'localhost'`
+	createUserSQL = `CREATE USER 'test2'@'localhost'`
+	tk.Exec(createUserSQL)


use MustExec?

zz-jason

LGTM

shenli reviewed May 23, 2018

View reviewed changes

zhexuany force-pushed the fix_drop_user_bug branch from 7a806fc to 0af59be Compare May 23, 2018 07:58

alivxxx reviewed May 23, 2018

View reviewed changes

winkyao reviewed May 23, 2018

View reviewed changes

shenli reviewed May 24, 2018

View reviewed changes

tiancaiamao reviewed May 24, 2018

View reviewed changes

tiancaiamao reviewed May 25, 2018

View reviewed changes

zhexuany added 10 commits May 30, 2018 11:09

fix drop user bug

f00bcea

address reviewer's comment

a739674

address reviewer's comment

f95fcfa

delete privileges from mysql.tables_priv

00123ea

address reviewer's comment

7791b93

correct typo

602a275

use errors.New

3f5ec4a

avoid use restrictedSQL mode

2054568

address reviewer's comment

ba99296

remove debug code

89e5973

zhexuany force-pushed the fix_drop_user_bug branch from 64c3cd0 to 89e5973 Compare May 30, 2018 03:09

tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label May 30, 2018

shenli reviewed May 30, 2018

View reviewed changes

zz-jason reviewed Jun 4, 2018

View reviewed changes

address reviewer's comment

542dde5

zz-jason reviewed Jun 5, 2018

View reviewed changes

coocood added the request-change label Jun 5, 2018

rollback when meet failure during batch delete stmts

9c62513

shenli added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 6, 2018

add a test case when drop user meet failure

5cbbc29

zhexuany force-pushed the fix_drop_user_bug branch from 21f4494 to 5cbbc29 Compare June 6, 2018 07:40

add batch drop when meet error

1f05eef

zhexuany force-pushed the fix_drop_user_bug branch from d3b5b93 to 1f05eef Compare June 6, 2018 08:01

zz-jason reviewed Jun 6, 2018

View reviewed changes

use MustExec instead of exec

ad10072

zhexuany force-pushed the fix_drop_user_bug branch from c29daae to ad10072 Compare June 6, 2018 08:34

zz-jason approved these changes Jun 6, 2018

View reviewed changes

Merge branch 'master' into fix_drop_user_bug

57f410e

zhexuany merged commit 7227db3 into pingcap:master Jun 6, 2018

zhexuany deleted the fix_drop_user_bug branch June 6, 2018 09:13

tiancaiamao mentioned this pull request Jul 8, 2018

Show grants still keep result after user has been dropped #7007

Closed

zhexuany added a commit to zhexuany/tidb that referenced this pull request Jul 9, 2018

[cherry=pick] executor: fix drop user bug (pingcap#6624)

a6b5bf3

zhexuany mentioned this pull request Jul 9, 2018

executor: fix drop user bug (#6624) #7014

Merged

shenli pushed a commit that referenced this pull request Jul 9, 2018

[cherrypick-2.0] executor: fix drop user bug (#6624) (#7014)

cef80ce

executor: fix drop user bug #6624

executor: fix drop user bug #6624

Conversation

zhexuany commented May 23, 2018 • edited Loading

zhexuany commented May 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winkyao left a comment

Choose a reason for hiding this comment

zhexuany commented May 23, 2018

shenli commented May 23, 2018

zhexuany commented May 24, 2018

Choose a reason for hiding this comment

tiancaiamao left a 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

Choose a reason for hiding this comment

zhexuany commented May 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiancaiamao commented May 25, 2018

zhexuany commented May 30, 2018

zhexuany commented May 30, 2018

tiancaiamao commented May 30, 2018

Choose a reason for hiding this comment

zhexuany commented Jun 4, 2018

Choose a reason for hiding this comment

zhexuany commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shenli commented Jun 6, 2018

zhexuany commented Jun 6, 2018

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

zhexuany commented May 23, 2018 •

edited

Loading