From 9ba9feac3453949547ada803ff5b77484c34cb32 Mon Sep 17 00:00:00 2001 From: Tanner Date: Tue, 10 Sep 2019 21:42:52 +0800 Subject: [PATCH 1/2] ddl: disallow dropping auto_increment column attribute (#12107) --- ddl/db_integration_test.go | 6 +++++- 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 ++++++++ 6 files changed, 28 insertions(+), 1 deletion(-) diff --git a/ddl/db_integration_test.go b/ddl/db_integration_test.go index 505a44dff3522..4993ed9e84a0e 100644 --- a/ddl/db_integration_test.go +++ b/ddl/db_integration_test.go @@ -1556,11 +1556,15 @@ func (s *testIntegrationSuite4) TestAlterColumn(c *C) { 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" c.Assert(createSQL, Equals, expected) - s.tk.MustExec("alter table mc modify column a bigint") // Drops auto_increment + _, err = s.tk.Exec("alter table mc modify column a bigint") // Droppping auto_increment is not allow when @@tidb_allow_remove_auto_inc == 'off' + c.Assert(err, NotNil) + s.tk.MustExec("set @@tidb_allow_remove_auto_inc = on") + s.tk.MustExec("alter table mc modify column a bigint") // Dropping auto_increment is ok when @@tidb_allow_remove_auto_inc == 'on' 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" c.Assert(createSQL, Equals, expected) + _, err = s.tk.Exec("alter table mc modify column a bigint auto_increment") // Adds auto_increment should throw error c.Assert(err, NotNil) diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index 7000afe1fb320..f0438941462f2 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -2609,6 +2609,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 support modifying the type definitions of 'null' to 'not null' now. var modifyColumnTp byte diff --git a/sessionctx/variable/session.go b/sessionctx/variable/session.go index aa83a0c39954d..2ec70c5026a5a 100644 --- a/sessionctx/variable/session.go +++ b/sessionctx/variable/session.go @@ -392,6 +392,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. @@ -444,6 +447,7 @@ func NewSessionVars() *SessionVars { SlowQueryFile: config.GetGlobalConfig().Log.SlowQueryFile, WaitSplitRegionFinish: DefTiDBWaitSplitRegionFinish, WaitSplitRegionTimeout: DefWaitSplitRegionTimeout, + AllowRemoveAutoInc: DefTiDBAllowRemoveAutoInc, } vars.Concurrency = Concurrency{ IndexLookupConcurrency: DefIndexLookupConcurrency, @@ -825,6 +829,8 @@ func (s *SessionVars) SetSystemVar(name string, val string) error { s.TxnMode = strings.ToUpper(val) case TiDBLowResolutionTSO: s.LowResolutionTSO = TiDBOptOn(val) + 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 233484f99af45..c5632cf987e80 100644 --- a/sessionctx/variable/sysvar.go +++ b/sessionctx/variable/sysvar.go @@ -701,6 +701,7 @@ var defaultSysVars = []*SysVar{ {ScopeSession, TiDBWaitSplitRegionTimeout, strconv.Itoa(DefWaitSplitRegionTimeout)}, {ScopeSession, TiDBLowResolutionTSO, "0"}, {ScopeSession, TiDBExpensiveQueryTimeThreshold, strconv.Itoa(DefTiDBExpensiveQueryTimeThreshold)}, + {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 002ebf722b38a..46ebb8b17c8cb 100644 --- a/sessionctx/variable/tidb_vars.go +++ b/sessionctx/variable/tidb_vars.go @@ -142,6 +142,9 @@ const ( // TiDBLowResolutionTSO is used for reading data with low resolution TSO which is updated once every two seconds TiDBLowResolutionTSO = "tidb_low_resolution_tso" + + // 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. @@ -344,6 +347,7 @@ const ( DefTiDBWaitSplitRegionFinish = true DefTiDBExpensiveQueryTimeThreshold = 60 // 60s DefWaitSplitRegionTimeout = 300 // 300s + DefTiDBAllowRemoveAutoInc = false ) // Process global variables. diff --git a/sessionctx/variable/varsutil.go b/sessionctx/variable/varsutil.go index 8196b2315aa51..bf8261f661a53 100644 --- a/sessionctx/variable/varsutil.go +++ b/sessionctx/variable/varsutil.go @@ -569,6 +569,14 @@ func ValidateSetSystemVar(vars *SessionVars, name string, value string) (string, default: return value, ErrWrongValueForVar.GenWithStackByArgs(TiDBTxnMode, value) } + 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 098fd28ba8ace1d8234b000fc8629da61de9cf3e Mon Sep 17 00:00:00 2001 From: tangenta Date: Wed, 11 Sep 2019 14:45:52 +0800 Subject: [PATCH 2/2] gofmt --- sessionctx/variable/session.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sessionctx/variable/session.go b/sessionctx/variable/session.go index 1d126bc9cba96..f52decd4a6b2d 100644 --- a/sessionctx/variable/session.go +++ b/sessionctx/variable/session.go @@ -393,7 +393,7 @@ type SessionVars struct { // ConnectionInfo indicates current connection info used by current session, only be lazy assigned by plugin. ConnectionInfo *ConnectionInfo - // StartTime is the start time of the last query. + // StartTime is the start time of the last query. StartTime time.Time // DurationParse is the duration of pasing SQL string to AST of the last query. @@ -402,7 +402,7 @@ type SessionVars struct { // DurationCompile is the duration of compiling AST to execution plan of the last query. DurationCompile time.Duration - // AllowRemoveAutoInc indicates whether a user can drop the auto_increment column attribute or not. + // AllowRemoveAutoInc indicates whether a user can drop the auto_increment column attribute or not. AllowRemoveAutoInc bool }