From 105b6c5a272a74f02413b2aa413d2a125506103d Mon Sep 17 00:00:00 2001 From: Chao Wang Date: Tue, 28 Dec 2021 10:27:42 +0800 Subject: [PATCH 1/5] *: Remove variable `tidb_enable_alter_placement` --- ddl/db_test.go | 2 -- ddl/ddl_api.go | 3 --- ddl/error.go | 3 --- ddl/placement_sql_test.go | 10 +--------- ddl/serial_test.go | 6 +----- docs/design/2021-07-29-hidden-sysvars.md | 1 - executor/executor_test.go | 2 -- sessionctx/variable/sysvar.go | 4 ---- sessionctx/variable/tidb_vars.go | 3 --- sessionctx/variable/variable.go | 2 +- 10 files changed, 3 insertions(+), 33 deletions(-) diff --git a/ddl/db_test.go b/ddl/db_test.go index 309ca8a03e736..dab68ee0b1e4c 100644 --- a/ddl/db_test.go +++ b/ddl/db_test.go @@ -127,8 +127,6 @@ func setUpSuite(s *testDBSuite, c *C) { c.Assert(err, IsNil) _, err = s.s.Execute(context.Background(), "set @@global.tidb_max_delta_schema_count= 4096") c.Assert(err, IsNil) - _, err = s.s.Execute(context.Background(), "set @@global.tidb_enable_alter_placement=1") - c.Assert(err, IsNil) } func tearDownSuite(s *testDBSuite, c *C) { diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index 23cc306cd77b1..cf13a508f013c 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -236,9 +236,6 @@ func (d *ddl) AlterTablePlacement(ctx sessionctx.Context, ident ast.Ident, place } func checkAndNormalizePlacement(ctx sessionctx.Context, placementPolicyRef *model.PolicyRefInfo, directPlacementOpts *model.PlacementSettings, fallbackPlacementPolicyRef *model.PolicyRefInfo, fallbackDirectPlacementOpts *model.PlacementSettings) (*model.PolicyRefInfo, *model.PlacementSettings, error) { - if !ctx.GetSessionVars().EnableAlterPlacement && (placementPolicyRef != nil || directPlacementOpts != nil) { - return nil, nil, ErrPlacementDisabled - } if placementPolicyRef != nil && directPlacementOpts != nil { return nil, nil, errors.Trace(ErrPlacementPolicyWithDirectOption.GenWithStackByArgs(placementPolicyRef.Name)) } diff --git a/ddl/error.go b/ddl/error.go index 6819920860e39..955cfbdbfa589 100644 --- a/ddl/error.go +++ b/ddl/error.go @@ -269,9 +269,6 @@ var ( // ErrPlacementPolicyInUse is returned when placement policy is in use in drop/alter. ErrPlacementPolicyInUse = dbterror.ClassDDL.NewStd(mysql.ErrPlacementPolicyInUse) - // ErrPlacementDisabled is returned when tidb_enable_alter_placement = 0 - ErrPlacementDisabled = dbterror.ClassDDL.NewStdErr(mysql.ErrUnsupportedDDLOperation, parser_mysql.Message("Alter Placement Rule is disabled, please set 'tidb_enable_alter_placement' if you need to enable it", nil)) - // ErrMultipleDefConstInListPart returns multiple definition of same constant in list partitioning. ErrMultipleDefConstInListPart = dbterror.ClassDDL.NewStd(mysql.ErrMultipleDefConstInListPart) diff --git a/ddl/placement_sql_test.go b/ddl/placement_sql_test.go index 7dd419bac54fc..de032c9eb476e 100644 --- a/ddl/placement_sql_test.go +++ b/ddl/placement_sql_test.go @@ -15,7 +15,6 @@ package ddl_test import ( - "context" "fmt" "sort" @@ -450,8 +449,6 @@ func (s *testDBSuite6) TestEnablePlacementCheck(c *C) { tk := testkit.NewTestKit(c, s.store) se, err := session.CreateSession4Test(s.store) c.Assert(err, IsNil) - _, err = se.Execute(context.Background(), "set @@global.tidb_enable_alter_placement=1") - c.Assert(err, IsNil) tk.MustExec("drop database if exists TestPlacementDB;") tk.MustExec("create database TestPlacementDB;") @@ -476,11 +473,6 @@ func (s *testDBSuite6) TestEnablePlacementCheck(c *C) { func (s *testDBSuite6) TestPlacementTiflashCheck(c *C) { tk := testkit.NewTestKit(c, s.store) - se, err := session.CreateSession4Test(s.store) - c.Assert(err, IsNil) - _, err = se.Execute(context.Background(), "set @@global.tidb_enable_alter_placement=1") - c.Assert(err, IsNil) - c.Assert(failpoint.Enable("github.com/pingcap/tidb/infoschema/mockTiFlashStoreCount", `return(true)`), IsNil) defer func() { err := failpoint.Disable("github.com/pingcap/tidb/infoschema/mockTiFlashStoreCount") @@ -501,7 +493,7 @@ func (s *testDBSuite6) TestPlacementTiflashCheck(c *C) { defer tk.MustExec("drop table if exists tp") tk.MustExec("alter table tp set tiflash replica 1") - err = tk.ExecToErr("alter table tp placement policy p1") + err := tk.ExecToErr("alter table tp placement policy p1") c.Assert(ddl.ErrIncompatibleTiFlashAndPlacement.Equal(err), IsTrue) err = tk.ExecToErr("alter table tp primary_region='r2' regions='r2'") c.Assert(ddl.ErrIncompatibleTiFlashAndPlacement.Equal(err), IsTrue) diff --git a/ddl/serial_test.go b/ddl/serial_test.go index 51dccd346b336..8811dfac12558 100644 --- a/ddl/serial_test.go +++ b/ddl/serial_test.go @@ -530,17 +530,13 @@ func (s *testSerialSuite) TestCreateTableWithLike(c *C) { func (s *testSerialSuite) TestCreateTableWithLikeAtTemporaryMode(c *C) { tk := testkit.NewTestKit(c, s.store) - se, err := session.CreateSession4Test(s.store) - c.Assert(err, IsNil) - _, err = se.Execute(context.Background(), "set @@global.tidb_enable_alter_placement=1") - c.Assert(err, IsNil) // Test create table like at temporary mode. tk.MustExec("use test") tk.MustExec("drop table if exists temporary_table;") tk.MustExec("create global temporary table temporary_table (a int, b int,index(a)) on commit delete rows") tk.MustExec("drop table if exists temporary_table_t1;") - _, err = tk.Exec("create table temporary_table_t1 like temporary_table") + _, err := tk.Exec("create table temporary_table_t1 like temporary_table") c.Assert(err.Error(), Equals, core.ErrOptOnTemporaryTable.GenWithStackByArgs("create table like").Error()) tk.MustExec("drop table if exists temporary_table;") diff --git a/docs/design/2021-07-29-hidden-sysvars.md b/docs/design/2021-07-29-hidden-sysvars.md index 59989da7a7099..cf46803f644be 100644 --- a/docs/design/2021-07-29-hidden-sysvars.md +++ b/docs/design/2021-07-29-hidden-sysvars.md @@ -39,7 +39,6 @@ There are currently ~20 hidden system variables: | tidb_enable_pipelined_window_function | ‘Pipelined window function’ feature | #1 Stable | | tidb_enable_change_multi_schema | ‘Change multi schema in one statement’ feature. | #3 In Development Feature | | tidb_enable_point_get_cache | ‘point get cache’ feature | #2 Experimental | -| tidb_enable_alter_placement | placement rules in SQL feature | #3 In Development Feature | | tidb_enable_extended_stats | ‘extended stats’ feature | #2 Experimental | | tidb_partition_prune_mode | Is partition prune mode dynamic or static | #2 Experimental | | tidb_enable_async_commit | Support Async Commit PRD | #1 Stable | diff --git a/executor/executor_test.go b/executor/executor_test.go index d4b11b0758d8b..ecc013e25ae4e 100644 --- a/executor/executor_test.go +++ b/executor/executor_test.go @@ -208,8 +208,6 @@ func (s *baseTestSuite) SetUpSuite(c *C) { c.Assert(err, IsNil) se, err := session.CreateSession4Test(s.store) c.Assert(err, IsNil) - _, err = se.Execute(context.Background(), "set @@global.tidb_enable_alter_placement=1") - c.Assert(err, IsNil) se.Close() d.SetStatsUpdating(true) s.domain = d diff --git a/sessionctx/variable/sysvar.go b/sessionctx/variable/sysvar.go index 396c93bddba1b..53150dfe93a47 100644 --- a/sessionctx/variable/sysvar.go +++ b/sessionctx/variable/sysvar.go @@ -852,10 +852,6 @@ var defaultSysVars = []*SysVar{ s.EnablePointGetCache = TiDBOptOn(val) return nil }}, - {Scope: ScopeGlobal, Name: TiDBEnableAlterPlacement, Value: BoolToOnOff(DefTiDBEnableAlterPlacement), Hidden: true, Type: TypeBool, SetSession: func(s *SessionVars, val string) error { - s.EnableAlterPlacement = TiDBOptOn(val) - return nil - }}, {Scope: ScopeSession, Name: TiDBForcePriority, skipInit: true, Value: mysql.Priority2Str[DefTiDBForcePriority], SetSession: func(s *SessionVars, val string) error { atomic.StoreInt32(&ForcePriority, int32(mysql.Str2Priority(val))) return nil diff --git a/sessionctx/variable/tidb_vars.go b/sessionctx/variable/tidb_vars.go index 7540000f4fc8b..45e2aec914029 100644 --- a/sessionctx/variable/tidb_vars.go +++ b/sessionctx/variable/tidb_vars.go @@ -410,9 +410,6 @@ const ( // TiDBEnablePointGetCache is used to control whether to enable the point get cache for special scenario. TiDBEnablePointGetCache = "tidb_enable_point_get_cache" - // TiDBEnableAlterPlacement is used to control whether to enable alter table partition. - TiDBEnableAlterPlacement = "tidb_enable_alter_placement" - // tidb_max_delta_schema_count defines the max length of deltaSchemaInfos. // deltaSchemaInfos is a queue that maintains the history of schema changes. TiDBMaxDeltaSchemaCount = "tidb_max_delta_schema_count" diff --git a/sessionctx/variable/variable.go b/sessionctx/variable/variable.go index aeb65a8257d9f..d8f2e0cf87101 100644 --- a/sessionctx/variable/variable.go +++ b/sessionctx/variable/variable.go @@ -497,7 +497,7 @@ func (sv *SysVar) SkipInit() bool { // These a special "Global-only" sysvars that for backward compatibility // are currently cached in the session. Please don't add to this list. switch sv.Name { - case TiDBEnableChangeMultiSchema, TiDBDDLReorgBatchSize, TiDBEnableAlterPlacement, + case TiDBEnableChangeMultiSchema, TiDBDDLReorgBatchSize, TiDBMaxDeltaSchemaCount, InitConnect, MaxPreparedStmtCount, TiDBDDLReorgWorkerCount, TiDBDDLErrorCountLimit, TiDBRowFormatVersion, TiDBEnableTelemetry, TiDBEnablePointGetCache: From 2994929cb706902182e8df4bb8ed82ecb7d8befc Mon Sep 17 00:00:00 2001 From: Chao Wang Date: Tue, 28 Dec 2021 10:55:40 +0800 Subject: [PATCH 2/5] some modify --- sessionctx/variable/session.go | 4 ---- sessionctx/variable/sysvar_test.go | 2 +- sessionctx/variable/tidb_vars.go | 1 - 3 files changed, 1 insertion(+), 6 deletions(-) diff --git a/sessionctx/variable/session.go b/sessionctx/variable/session.go index b205bda1de5ab..874884375bee8 100644 --- a/sessionctx/variable/session.go +++ b/sessionctx/variable/session.go @@ -719,9 +719,6 @@ type SessionVars struct { // EnablePointGetCache is used to cache value for point get for read only scenario. EnablePointGetCache bool - // EnableAlterPlacement indicates whether a user can alter table partition placement rules. - EnableAlterPlacement bool - // EnablePlacementChecks indicates whether a user can check validation of placement. EnablePlacementChecks bool @@ -1197,7 +1194,6 @@ func NewSessionVars() *SessionVars { ShardAllocateStep: DefTiDBShardAllocateStep, EnableChangeMultiSchema: DefTiDBChangeMultiSchema, EnablePointGetCache: DefTiDBPointGetCache, - EnableAlterPlacement: DefTiDBEnableAlterPlacement, EnableAmendPessimisticTxn: DefTiDBEnableAmendPessimisticTxn, PartitionPruneMode: *atomic2.NewString(DefTiDBPartitionPruneMode), TxnScope: kv.NewDefaultTxnScopeVar(), diff --git a/sessionctx/variable/sysvar_test.go b/sessionctx/variable/sysvar_test.go index 91b3451f15ed9..da110dc571529 100644 --- a/sessionctx/variable/sysvar_test.go +++ b/sessionctx/variable/sysvar_test.go @@ -668,7 +668,7 @@ func TestSettersandGetters(t *testing.T) { // There are some historial exceptions where global variables are loaded into the session. // Please don't add to this list, the behavior is not MySQL compatible. switch sv.Name { - case TiDBEnableChangeMultiSchema, TiDBDDLReorgBatchSize, TiDBEnableAlterPlacement, + case TiDBEnableChangeMultiSchema, TiDBDDLReorgBatchSize, TiDBMaxDeltaSchemaCount, InitConnect, MaxPreparedStmtCount, TiDBDDLReorgWorkerCount, TiDBDDLErrorCountLimit, TiDBRowFormatVersion, TiDBEnableTelemetry, TiDBEnablePointGetCache: diff --git a/sessionctx/variable/tidb_vars.go b/sessionctx/variable/tidb_vars.go index 45e2aec914029..b64be2e150d03 100644 --- a/sessionctx/variable/tidb_vars.go +++ b/sessionctx/variable/tidb_vars.go @@ -710,7 +710,6 @@ const ( DefTiDBMaxDeltaSchemaCount = 1024 DefTiDBChangeMultiSchema = false DefTiDBPointGetCache = false - DefTiDBEnableAlterPlacement = false DefTiDBEnableAutoIncrementInGenerated = false DefTiDBHashAggPartialConcurrency = ConcurrencyUnset DefTiDBHashAggFinalConcurrency = ConcurrencyUnset From f8a67b4d9d732c9a2385e196134e9c19934ca042 Mon Sep 17 00:00:00 2001 From: Chao Wang Date: Tue, 28 Dec 2021 12:19:15 +0800 Subject: [PATCH 3/5] modify some tests --- ddl/placement_sql_test.go | 11 ----------- privilege/privileges/privileges_test.go | 1 - 2 files changed, 12 deletions(-) diff --git a/ddl/placement_sql_test.go b/ddl/placement_sql_test.go index de032c9eb476e..0a2fe66dedc19 100644 --- a/ddl/placement_sql_test.go +++ b/ddl/placement_sql_test.go @@ -33,13 +33,11 @@ import ( func (s *testDBSuite1) TestPlacementPolicyCache(c *C) { tk := testkit.NewTestKit(c, s.store) tk.MustExec("use test") - tk.Se.GetSessionVars().EnableAlterPlacement = true tk.MustExec("set @@tidb_enable_exchange_partition = 1") defer func() { tk.MustExec("set @@tidb_enable_exchange_partition = 0") tk.MustExec("drop table if exists t1") tk.MustExec("drop table if exists t2") - tk.Se.GetSessionVars().EnableAlterPlacement = false }() initTable := func() []string { @@ -102,10 +100,8 @@ func (s *testSerialDBSuite) TestTxnScopeConstraint(c *C) { tk := testkit.NewTestKit(c, s.store) tk.MustExec("use test") tk.MustExec("drop table if exists t1") - tk.Se.GetSessionVars().EnableAlterPlacement = true defer func() { tk.MustExec("drop table if exists t1") - tk.Se.GetSessionVars().EnableAlterPlacement = false }() tk.MustExec(`create table t1 (c int) @@ -256,13 +252,11 @@ func (s *testDBSuite6) TestCreateSchemaWithPlacement(c *C) { tk := testkit.NewTestKit(c, s.store) tk.MustExec("drop schema if exists SchemaDirectPlacementTest") tk.MustExec("drop schema if exists SchemaPolicyPlacementTest") - tk.Se.GetSessionVars().EnableAlterPlacement = true defer func() { tk.MustExec("drop schema if exists SchemaDirectPlacementTest") tk.MustExec("drop schema if exists SchemaPolicyPlacementTest") tk.MustExec("drop placement policy if exists PolicySchemaTest") tk.MustExec("drop placement policy if exists PolicyTableTest") - tk.Se.GetSessionVars().EnableAlterPlacement = false }() tk.MustExec(`CREATE SCHEMA SchemaDirectPlacementTest PRIMARY_REGION='nl' REGIONS = "se,nz,nl" FOLLOWERS=3`) @@ -447,22 +441,17 @@ func (s *testDBSuite6) TestAlterDBPlacement(c *C) { func (s *testDBSuite6) TestEnablePlacementCheck(c *C) { tk := testkit.NewTestKit(c, s.store) - se, err := session.CreateSession4Test(s.store) - c.Assert(err, IsNil) - tk.MustExec("drop database if exists TestPlacementDB;") tk.MustExec("create database TestPlacementDB;") tk.MustExec("use TestPlacementDB;") tk.MustExec("drop placement policy if exists placement_x;") tk.MustExec("create placement policy placement_x PRIMARY_REGION=\"cn-east-1\", REGIONS=\"cn-east-1\";") - se.GetSessionVars().EnableAlterPlacement = true tk.MustExec("create table t(c int) partition by range (c) (partition p1 values less than (200) followers=2);") defer func() { tk.MustExec("drop database if exists TestPlacementDB;") tk.MustExec("drop placement policy if exists placement_x;") }() - tk.Se.GetSessionVars().EnableAlterPlacement = false tk.MustGetErrCode("create database TestPlacementDB2 followers=2;", mysql.ErrUnsupportedDDLOperation) tk.MustGetErrCode("alter database TestPlacementDB placement policy=placement_x", mysql.ErrUnsupportedDDLOperation) tk.MustGetErrCode("create table t (c int) FOLLOWERS=2;", mysql.ErrUnsupportedDDLOperation) diff --git a/privilege/privileges/privileges_test.go b/privilege/privileges/privileges_test.go index d9b7d94b64b33..ba68d74860944 100644 --- a/privilege/privileges/privileges_test.go +++ b/privilege/privileges/privileges_test.go @@ -2595,7 +2595,6 @@ func TestInformationSchemaPlacmentRulesPrivileges(t *testing.T) { }() tk.MustExec("CREATE DATABASE placement_rule_db") tk.MustExec("USE placement_rule_db") - tk.Session().GetSessionVars().EnableAlterPlacement = true tk.MustExec(`CREATE TABLE placement_rule_table_se (a int) PRIMARY_REGION="se" REGIONS="se,nl"`) tk.MustExec(`CREATE TABLE placement_rule_table_nl (a int) PRIMARY_REGION="nl" REGIONS="se,nl"`) tk.MustQuery(`SELECT * FROM information_schema.placement_rules WHERE SCHEMA_NAME = "placement_rule_db"`).Sort().Check(testkit.Rows( From 7310bdbb3385d920e94ab4b3d88ccea167ebcc16 Mon Sep 17 00:00:00 2001 From: Chao Wang Date: Tue, 28 Dec 2021 13:07:11 +0800 Subject: [PATCH 4/5] update ut --- ddl/placement_sql_test.go | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/ddl/placement_sql_test.go b/ddl/placement_sql_test.go index 0a2fe66dedc19..24bc343016ff2 100644 --- a/ddl/placement_sql_test.go +++ b/ddl/placement_sql_test.go @@ -438,28 +438,6 @@ func (s *testDBSuite6) TestAlterDBPlacement(c *C) { )) } -func (s *testDBSuite6) TestEnablePlacementCheck(c *C) { - - tk := testkit.NewTestKit(c, s.store) - tk.MustExec("drop database if exists TestPlacementDB;") - tk.MustExec("create database TestPlacementDB;") - tk.MustExec("use TestPlacementDB;") - tk.MustExec("drop placement policy if exists placement_x;") - tk.MustExec("create placement policy placement_x PRIMARY_REGION=\"cn-east-1\", REGIONS=\"cn-east-1\";") - tk.MustExec("create table t(c int) partition by range (c) (partition p1 values less than (200) followers=2);") - defer func() { - tk.MustExec("drop database if exists TestPlacementDB;") - tk.MustExec("drop placement policy if exists placement_x;") - }() - - tk.MustGetErrCode("create database TestPlacementDB2 followers=2;", mysql.ErrUnsupportedDDLOperation) - tk.MustGetErrCode("alter database TestPlacementDB placement policy=placement_x", mysql.ErrUnsupportedDDLOperation) - tk.MustGetErrCode("create table t (c int) FOLLOWERS=2;", mysql.ErrUnsupportedDDLOperation) - tk.MustGetErrCode("alter table t voters=2;", mysql.ErrUnsupportedDDLOperation) - tk.MustGetErrCode("create table m (c int) partition by range (c) (partition p1 values less than (200) followers=2);", mysql.ErrUnsupportedDDLOperation) - tk.MustGetErrCode("alter table t partition p1 placement policy=\"placement_x\";", mysql.ErrUnsupportedDDLOperation) -} - func (s *testDBSuite6) TestPlacementTiflashCheck(c *C) { tk := testkit.NewTestKit(c, s.store) c.Assert(failpoint.Enable("github.com/pingcap/tidb/infoschema/mockTiFlashStoreCount", `return(true)`), IsNil) From f3c4c3fee722111d39e759a6cf8f68727f4739a7 Mon Sep 17 00:00:00 2001 From: Chao Wang Date: Thu, 30 Dec 2021 10:21:08 +0800 Subject: [PATCH 5/5] address comments --- sessionctx/variable/removed.go | 1 + sessionctx/variable/tidb_vars.go | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/sessionctx/variable/removed.go b/sessionctx/variable/removed.go index 45c2e1704a02f..e3ae4556e6d85 100644 --- a/sessionctx/variable/removed.go +++ b/sessionctx/variable/removed.go @@ -22,6 +22,7 @@ package variable // careful not to return dummy data. var removedSysVars = map[string]string{ + TiDBEnableAlterPlacement: "alter placement is now always enabled", TiDBEnableGlobalTemporaryTable: "temporary table support is now always enabled", TiDBSlowLogMasking: "use tidb_redact_log instead", } diff --git a/sessionctx/variable/tidb_vars.go b/sessionctx/variable/tidb_vars.go index b64be2e150d03..b32ea22b6150d 100644 --- a/sessionctx/variable/tidb_vars.go +++ b/sessionctx/variable/tidb_vars.go @@ -410,6 +410,10 @@ const ( // TiDBEnablePointGetCache is used to control whether to enable the point get cache for special scenario. TiDBEnablePointGetCache = "tidb_enable_point_get_cache" + // TiDBEnableAlterPlacement is used to control whether to enable alter table partition. + // Deprecated: It is removed and do not use it again + TiDBEnableAlterPlacement = "tidb_enable_alter_placement" + // tidb_max_delta_schema_count defines the max length of deltaSchemaInfos. // deltaSchemaInfos is a queue that maintains the history of schema changes. TiDBMaxDeltaSchemaCount = "tidb_max_delta_schema_count"