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

executor: forbid user to drop important system table #7471

Merged
merged 8 commits into from Aug 29, 2018

Conversation

tiancaiamao
Copy link
Contributor

@tiancaiamao tiancaiamao commented Aug 23, 2018

What problem does this PR solve?

If a user drop some important tables by mistake, such as mysql.user, it's difficult to recover.
In the worst case, the whole cluster fails to bootstrap, we should prevent that.

What is changed and how it works?

Forbid user to drop important system tables.

Check List

Tests

  • Unit test

Side effects

It's harder to drop those tables.

Should we also protect the statistic tables? @lamxTyler
@zimulala @shenli

If a user drop some important tables by mistake, such as mysql.user,
it's difficult to recover.
In the worst case, the whole cluster fails to bootstrap, we should
prevent that.
@@ -198,6 +230,14 @@ func (e *DDLExec) executeDropTable(s *ast.DropTableStmt) error {
return errors.Trace(err)
}

// 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 isSystemTable(tn.Schema.L, tn.Name.L) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about forbidding drop any tables in database mysql? It can avoid we add some important table in mysql, and forget to modify the systemTables map.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's your opinion? @shenli @lamxTyler

Copy link
Member

Choose a reason for hiding this comment

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

I suggest protecting all the tables in the mysql database.

executor/ddl.go Outdated
// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need to consider the privileges.SkipWithGrant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know it's a rare case, but how about that we really want to do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need a "backdoor".

@@ -198,6 +230,14 @@ func (e *DDLExec) executeDropTable(s *ast.DropTableStmt) error {
return errors.Trace(err)
}

// 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 isSystemTable(tn.Schema.L, tn.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.

I suggest protecting all the tables in the mysql database.

executor/ddl.go Outdated
@@ -154,8 +155,19 @@ func (e *DDLExec) executeCreateIndex(s *ast.CreateIndexStmt) error {
return errors.Trace(err)
}

var errForbidDrop = errors.New("Hey, drop system database/table, are you sure?")
Copy link
Member

Choose a reason for hiding this comment

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

Please make the error message more seriously.

"gc_delete_range_done": {},
}

func isSystemTable(schema, table string) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we put it to infoschema.go? I think it is like "IsMemoryDB", we can put these together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is removed, we forbid dropping tables in "mysql" database.

Copy link
Member

@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.

If we forbid drop all the tables in the database "mysql", I think the user will blame that he cannot drop the table which he created in the database "mysql", except using skip_grand_table which requires to restart the tidb server.

@tiancaiamao
Copy link
Contributor Author

A sane person will not drop system tables.
A sane person will not create table on "mysql" database
This change tries to prevent some insane person from breaking the system, it has very small affect on a sane person.

@morgo
Copy link
Contributor

morgo commented Aug 24, 2018

I think this will break mysqldump restores. Here is an example of what mysqldump with --all-databases from 5.7 produces:

--
-- Table structure for table `user`
--

DROP TABLE IF EXISTS `user`;
/*!40101 SET @saved_cs_client     = @@character_set_client */;
/*!40101 SET character_set_client = utf8 */;
CREATE TABLE `user` (
  `Host` char(60) COLLATE utf8_bin NOT NULL DEFAULT '',
  `User` char(32) COLLATE utf8_bin NOT NULL DEFAULT '',
  `Select_priv` enum('N','Y') CHARACTER SET utf8 NOT NULL DEFAULT 'N',
  `Insert_priv` enum('N','Y') CHARACTER SET utf8 NOT NULL DEFAULT 'N',
..

@jackysp
Copy link
Member

jackysp commented Aug 24, 2018

"A sane person will not create table on "mysql" database" ? Maybe s/sane/genius/ will be all right :-)

@shenli
Copy link
Member

shenli commented Aug 26, 2018

@morgo Some tables in the mysql database are critical to TiDB. For example tidb, 'gc_*'. Users should never touch these tables.

@tiancaiamao User, '*_Priv' tables are not so critical. We could save the cluster even they are dropped. I think we could relax the protection for them.

@tiancaiamao
Copy link
Contributor Author

PTAL @shenli @jackysp

Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 27, 2018
@tiancaiamao
Copy link
Contributor Author

PTAL @shenli

Copy link
Member

@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 status/LGT2 Indicates that a PR has LGTM 2. require-LGT3 Indicates that the PR requires three LGTM. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 28, 2018
@tiancaiamao
Copy link
Contributor Author

/run-all-tests

@tiancaiamao
Copy link
Contributor Author

PTAL @shenli

executor/ddl.go Outdated
// 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 isSystemTable(tn.Schema.L, tn.Name.L) {
return errors.Trace(errForbidDrop)
Copy link
Member

Choose a reason for hiding this comment

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

Add the system table name in the error message.

executor/ddl.go Outdated
@@ -179,6 +188,22 @@ func (e *DDLExec) executeDropDatabase(s *ast.DropDatabaseStmt) error {
return errors.Trace(err)
}

var systemTables = map[string]struct{}{
Copy link
Member

Choose a reason for hiding this comment

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

Add a comment for this map. Explain why we prevent users from dropping them.

@tiancaiamao
Copy link
Contributor Author

PTAL @shenli
/run-all-tests

Copy link
Member

@shenli shenli left a comment

Choose a reason for hiding this comment

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

LGTM

@shenli shenli added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Aug 29, 2018
@jackysp jackysp merged commit 55af7e1 into pingcap:master Aug 29, 2018
@tiancaiamao tiancaiamao deleted the forbit-drop-system-table branch August 29, 2018 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/privilege require-LGT3 Indicates that the PR requires three LGTM. status/LGT3 The PR has already had 3 LGTM. type/enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants