address comment

pingcap · Aug 27, 2018 · a9f82e6 · a9f82e6
1 parent ad05169
commit a9f82e6
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 16 deletions.
diff --git a/executor/ddl.go b/executor/ddl.go
@@ -24,7 +24,6 @@ import (
 	"github.com/pingcap/tidb/infoschema"
 	"github.com/pingcap/tidb/model"
 	"github.com/pingcap/tidb/mysql"
-	"github.com/pingcap/tidb/privilege/privileges"
 	"github.com/pingcap/tidb/sessionctx/variable"
 	"github.com/pingcap/tidb/types"
 	"github.com/pingcap/tidb/util/chunk"
@@ -155,17 +154,15 @@ func (e *DDLExec) executeCreateIndex(s *ast.CreateIndexStmt) error {
 	return errors.Trace(err)
 }
 
-var errForbidDrop = errors.New("Drop mysql tables is forbidden")
+var errForbidDrop = errors.New("Drop tidb system tables is forbidden")
 
 func (e *DDLExec) executeDropDatabase(s *ast.DropDatabaseStmt) error {
 	dbName := model.NewCIStr(s.Name)
 
 	// Protect important system table from been dropped by a mistake.
 	// I can hardly find a case that a user really need to do this.
 	if dbName.L == "mysql" {
-		if !privileges.SkipWithGrant {
-			return errors.Trace(errForbidDrop)
-		}
+		return errors.Trace(errForbidDrop)
 	}
 
 	err := domain.GetDomain(e.ctx).DDL().DropSchema(e.ctx, dbName)
@@ -191,6 +188,22 @@ func (e *DDLExec) executeDropDatabase(s *ast.DropDatabaseStmt) error {
 	return errors.Trace(err)
 }
 
+var systemTables = map[string]struct{}{
+	"tidb":                 {},
+	"gc_delete_range":      {},
+	"gc_delete_range_done": {},
+}
+
+func isSystemTable(schema, table string) bool {
+	if schema != "mysql" {
+		return false
+	}
+	if _, ok := systemTables[table]; ok {
+		return true
+	}
+	return false
+}
+
 func (e *DDLExec) executeDropTable(s *ast.DropTableStmt) error {
 	var notExistTables []string
 	for _, tn := range s.Tables {
@@ -212,10 +225,8 @@ func (e *DDLExec) executeDropTable(s *ast.DropTableStmt) error {
 
 		// Protect important system table from been dropped by a mistake.
 		// I can hardly find a case that a user really need to do this.
-		if tn.Schema.L == "mysql" {
-			if !privileges.SkipWithGrant {
-				return errors.Trace(errForbidDrop)
-			}
+		if isSystemTable(tn.Schema.L, tn.Name.L) {
+			return errors.Trace(errForbidDrop)
 		}
 
 		if config.CheckTableBeforeDrop {

diff --git a/executor/ddl_test.go b/executor/ddl_test.go
@@ -141,7 +141,7 @@ func (s *testSuite) TestCreateDropTable(c *C) {
 	tk.MustExec("create table drop_test (a int)")
 	tk.MustExec("drop table drop_test")
 
-	_, err := tk.Exec("drop table mysql.user")
+	_, err := tk.Exec("drop table mysql.gc_delete_range")
 	c.Assert(err, NotNil)
 }
 

diff --git a/privilege/privileges/cache_test.go b/privilege/privileges/cache_test.go
@@ -200,12 +200,6 @@ func (s *testCacheSuite) TestAbnormalMySQLTable(c *C) {
 	c.Assert(err, IsNil)
 	defer se.Close()
 
-	saveSkipWithGrant := privileges.SkipWithGrant
-	defer func() {
-		privileges.SkipWithGrant = saveSkipWithGrant
-	}()
-	privileges.SkipWithGrant = true
-
 	// Simulate the case mysql.user is synchronized from MySQL.
 	mustExec(c, se, "DROP TABLE mysql.user;")
 	mustExec(c, se, "USE mysql;")