From 493acc6a7f9f333bf45a0e66057ddf4dd76394da Mon Sep 17 00:00:00 2001 From: Tanner Date: Tue, 10 Sep 2019 21:42:52 +0800 Subject: [PATCH 1/3] ddl: disallow dropping auto_increment column attribute (#12107) --- ddl/ddl_api.go | 4 ++++ sessionctx/variable/session.go | 6 ++++++ sessionctx/variable/sysvar.go | 1 + sessionctx/variable/tidb_vars.go | 4 ++++ sessionctx/variable/varsutil.go | 8 ++++++++ 5 files changed, 23 insertions(+) diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index c4649b069eda6..6c55317e14b44 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -2091,6 +2091,10 @@ func (d *ddl) getModifiableColumnJob(ctx sessionctx.Context, ident ast.Ident, or if !mysql.HasAutoIncrementFlag(col.Flag) && mysql.HasAutoIncrementFlag(newCol.Flag) { return nil, errUnsupportedModifyColumn.GenWithStackByArgs("set auto_increment") } + // Disallow modifying column from auto_increment to not auto_increment if the session variable `AllowRemoveAutoInc` is false. + if !ctx.GetSessionVars().AllowRemoveAutoInc && mysql.HasAutoIncrementFlag(col.Flag) && !mysql.HasAutoIncrementFlag(newCol.Flag) { + return nil, errUnsupportedModifyColumn.GenWithStackByArgs("to remove auto_increment without @@tidb_allow_remove_auto_inc enabled") + } // We don't support modifying the type definitions from 'null' to 'not null' now. if !mysql.HasNotNullFlag(col.Flag) && mysql.HasNotNullFlag(newCol.Flag) { diff --git a/sessionctx/variable/session.go b/sessionctx/variable/session.go index c7321432949da..d7e325512305e 100644 --- a/sessionctx/variable/session.go +++ b/sessionctx/variable/session.go @@ -336,6 +336,9 @@ type SessionVars struct { // ConnectionInfo indicates current connection info used by current session, only be lazy assigned by plugin. ConnectionInfo *ConnectionInfo + + // AllowRemoveAutoInc indicates whether a user can drop the auto_increment column attribute or not. + AllowRemoveAutoInc bool } // ConnectionInfo present connection used by audit. @@ -381,6 +384,7 @@ func NewSessionVars() *SessionVars { WaitSplitRegionFinish: DefTiDBWaitSplitRegionFinish, WaitSplitRegionTimeout: DefWaitSplitRegionTimeout, CommandValue: uint32(mysql.ComSleep), + AllowRemoveAutoInc: DefTiDBAllowRemoveAutoInc, } vars.Concurrency = Concurrency{ IndexLookupConcurrency: DefIndexLookupConcurrency, @@ -675,6 +679,8 @@ func (s *SessionVars) SetSystemVar(name string, val string) error { s.WaitSplitRegionFinish = TiDBOptOn(val) case TiDBWaitSplitRegionTimeout: s.WaitSplitRegionTimeout = uint64(tidbOptPositiveInt32(val, DefWaitSplitRegionTimeout)) + case TiDBAllowRemoveAutoInc: + s.AllowRemoveAutoInc = TiDBOptOn(val) } s.systems[name] = val return nil diff --git a/sessionctx/variable/sysvar.go b/sessionctx/variable/sysvar.go index 535c1f1ccb7f6..40cf4693fc47c 100644 --- a/sessionctx/variable/sysvar.go +++ b/sessionctx/variable/sysvar.go @@ -680,6 +680,7 @@ var defaultSysVars = []*SysVar{ {ScopeGlobal, TiDBScatterRegion, BoolToIntStr(DefTiDBScatterRegion)}, {ScopeSession, TiDBWaitSplitRegionFinish, BoolToIntStr(DefTiDBWaitSplitRegionFinish)}, {ScopeSession, TiDBWaitSplitRegionTimeout, strconv.Itoa(DefWaitSplitRegionTimeout)}, + {ScopeSession, TiDBAllowRemoveAutoInc, BoolToIntStr(DefTiDBAllowRemoveAutoInc)}, } // SynonymsSysVariables is synonyms of system variables. diff --git a/sessionctx/variable/tidb_vars.go b/sessionctx/variable/tidb_vars.go index 94e9fba6385c9..a3ff0724da2c5 100644 --- a/sessionctx/variable/tidb_vars.go +++ b/sessionctx/variable/tidb_vars.go @@ -121,6 +121,9 @@ const ( // TiDBCheckMb4ValueInUTF8 is used to control whether to enable the check wrong utf8 value. TiDBCheckMb4ValueInUTF8 = "tidb_check_mb4_value_in_utf8" + + // TiDBAllowRemoveAutoInc indicates whether a user can drop the auto_increment column attribute or not. + TiDBAllowRemoveAutoInc = "tidb_allow_remove_auto_inc" ) // TiDB system variable names that both in session and global scope. @@ -270,6 +273,7 @@ const ( DefTiDBScatterRegion = false DefTiDBWaitSplitRegionFinish = true DefWaitSplitRegionTimeout = 300 // 300s + DefTiDBAllowRemoveAutoInc = false ) // Process global variables. diff --git a/sessionctx/variable/varsutil.go b/sessionctx/variable/varsutil.go index dda8d0dd92376..3bb01142bf929 100644 --- a/sessionctx/variable/varsutil.go +++ b/sessionctx/variable/varsutil.go @@ -390,6 +390,14 @@ func ValidateSetSystemVar(vars *SessionVars, name string, value string) (string, if v <= 0 { return value, errors.Errorf("tidb_wait_split_region_timeout(%d) cannot be smaller than 1", v) } + case TiDBAllowRemoveAutoInc: + switch { + case strings.EqualFold(value, "ON") || value == "1": + return "on", nil + case strings.EqualFold(value, "OFF") || value == "0": + return "off", nil + } + return value, ErrWrongValueForVar.GenWithStackByArgs(name, value) } return value, nil } From a4240ecc6b6338361ae13a3e20540a4c9562c33f Mon Sep 17 00:00:00 2001 From: tangenta Date: Wed, 11 Sep 2019 10:48:29 +0800 Subject: [PATCH 2/3] add test --- ddl/db_integration_test.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/ddl/db_integration_test.go b/ddl/db_integration_test.go index f669dea5c0c8a..0c259f637fa94 100644 --- a/ddl/db_integration_test.go +++ b/ddl/db_integration_test.go @@ -893,7 +893,7 @@ func (s *testIntegrationSuite) TestChangingDBCharset(c *C) { verifyDBCharsetAndCollate("alterdb2", "utf8mb4", "utf8mb4_general_ci") } -func (s *testIntegrationSuite) TestDropAutoIncrementIndex(c *C) { +func (s *testIntegrationSuite) TestDropAutoIncrement(c *C) { tk := testkit.NewTestKit(c, s.store) tk.MustExec("create database if not exists test") tk.MustExec("use test") @@ -914,4 +914,12 @@ func (s *testIntegrationSuite) TestDropAutoIncrementIndex(c *C) { tk.MustExec("create table t1 (a int(11) not null auto_increment, b int(11), c bigint, unique key (a, b, c))") dropIndexSQL = "alter table t1 drop index a" assertErrMsg(dropIndexSQL) + + tk.MustExec("drop table if exists t1") + tk.MustExec("create table t1 (a int auto_increment key)") + _, err := tk.Exec("alter table t modify column a int") + c.Assert(err, NotNil) + tk.MustExec("set @@tidb_allow_remove_auto_inc = on") + tk.MustExec("alter table t1 modify column a int") + tk.MustExec("set @@tidb_allow_remove_auto_inc = off") } From c367424fa9aa455fad49cde53b24b3c131295341 Mon Sep 17 00:00:00 2001 From: tangenta Date: Wed, 11 Sep 2019 14:55:47 +0800 Subject: [PATCH 3/3] fix test --- ddl/db_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ddl/db_test.go b/ddl/db_test.go index 8ae9ea61780c7..d997dd49c52a4 100644 --- a/ddl/db_test.go +++ b/ddl/db_test.go @@ -1885,7 +1885,9 @@ func (s *testDBSuite) TestAlterColumn(c *C) { result = s.tk.MustQuery("show create table mc") createSQL = result.Rows()[0][1] expected = "CREATE TABLE `mc` (\n `a` bigint(20) NOT NULL AUTO_INCREMENT,\n `b` int(11) DEFAULT NULL,\n PRIMARY KEY (`a`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin" + s.mustExec(c, "set @@tidb_allow_remove_auto_inc = on") s.mustExec(c, "alter table mc modify column a bigint") // Drops auto_increment + s.mustExec(c, "set @@tidb_allow_remove_auto_inc = off") result = s.tk.MustQuery("show create table mc") createSQL = result.Rows()[0][1] expected = "CREATE TABLE `mc` (\n `a` bigint(20) NOT NULL,\n `b` int(11) DEFAULT NULL,\n PRIMARY KEY (`a`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin"