From b7898adecd47fa7d4c5e297fdbcc8a92d2906ce4 Mon Sep 17 00:00:00 2001 From: BornChanger Date: Wed, 22 Feb 2023 19:43:33 +0800 Subject: [PATCH 01/12] *: support resource_group sql hint Signed-off-by: BornChanger --- ddl/resource_group_test.go | 18 ++++++++++++++++++ parser/ast/misc.go | 2 ++ parser/ast/misc_test.go | 2 ++ parser/hintparser.go | 6 ++++-- parser/hintparser.y | 6 ++++-- parser/hintparser_test.go | 6 +++++- parser/parser_test.go | 7 +++++-- planner/optimize.go | 22 +++++++++++++++++++++- session/session.go | 6 ++++++ sessionctx/stmtctx/stmtctx.go | 4 +++- 10 files changed, 70 insertions(+), 9 deletions(-) diff --git a/ddl/resource_group_test.go b/ddl/resource_group_test.go index 162a3ac233a05..40f4266a60e24 100644 --- a/ddl/resource_group_test.go +++ b/ddl/resource_group_test.go @@ -176,3 +176,21 @@ func testResourceGroupNameFromIS(t *testing.T, ctx sessionctx.Context, name stri g, _ := dom.InfoSchema().ResourceGroupByName(model.NewCIStr(name)) return g } + +func TestResourceGroupHint(t *testing.T) { + store, _ := testkit.CreateMockStoreAndDomain(t) + tk := testkit.NewTestKit(t, store) + + tk.MustExec("use test") + tk.MustExec("create table t1(c1 int)") + tk.MustExec("insert into t1 values(1)") + + tk.MustExec("set global tidb_enable_resource_control='on'") + tk.MustExec("create resource group rg1 ru_per_sec=1000") + tk.MustQuery("select /*+ resource_group(default) */ * from t1") + tk.MustQuery("select /*+ resource_group(rg1) */ * from t1") + tk.MustQuery("select /*+ resource_group(rg2) */ * from t1") + tk.MustQuery("select /*+ resource_group(rg1) */ * from information_schema.processlist").Check(testkit.Rows("1 test Sleep 0 autocommit select /*+ resource_group(rg1) */ * from information_schema.processlist 4b5e7cdd5d3ed84d6c1a6d56403a3d512554b534313caf296268abdec1c9ea99 0 0 rg1")) + tk.MustQuery("select * from information_schema.processlist").Check(testkit.Rows("1 test Sleep 0 autocommit select * from information_schema.processlist 4b5e7cdd5d3ed84d6c1a6d56403a3d512554b534313caf296268abdec1c9ea99 0 0 ")) + +} diff --git a/parser/ast/misc.go b/parser/ast/misc.go index 719371ee89439..5ba9f683c9bf6 100644 --- a/parser/ast/misc.go +++ b/parser/ast/misc.go @@ -3766,6 +3766,8 @@ func (n *TableOptimizerHint) Restore(ctx *format.RestoreCtx) error { switch n.HintName.L { case "max_execution_time": ctx.WritePlainf("%d", n.HintData.(uint64)) + case "resource_group": + ctx.WriteName(n.HintData.(string)) case "nth_plan": ctx.WritePlainf("%d", n.HintData.(int64)) case "tidb_hj", "tidb_smj", "tidb_inlj", "hash_join", "hash_join_build", "hash_join_probe", "merge_join", "inl_join", "broadcast_join", "shuffle_join", "inl_hash_join", "inl_merge_join", "leading": diff --git a/parser/ast/misc_test.go b/parser/ast/misc_test.go index 7379279d51567..4d046a8989831 100644 --- a/parser/ast/misc_test.go +++ b/parser/ast/misc_test.go @@ -303,6 +303,8 @@ func TestTableOptimizerHintRestore(t *testing.T) { {"READ_FROM_STORAGE(@sel TIFLASH[t1, t2])", "READ_FROM_STORAGE(@`sel` TIFLASH[`t1`, `t2`])"}, {"READ_FROM_STORAGE(@sel TIFLASH[t1 partition(p0)])", "READ_FROM_STORAGE(@`sel` TIFLASH[`t1` PARTITION(`p0`)])"}, {"TIME_RANGE('2020-02-02 10:10:10','2020-02-02 11:10:10')", "TIME_RANGE('2020-02-02 10:10:10', '2020-02-02 11:10:10')"}, + {"RESOURCE_GROUP(rg1)", "RESOURCE_GROUP(`rg1`)"}, + {"RESOURCE_GROUP(`default`)", "RESOURCE_GROUP(`default`)"}, } extractNodeFunc := func(node ast.Node) ast.Node { return node.(*ast.SelectStmt).TableHints[0] diff --git a/parser/hintparser.go b/parser/hintparser.go index 998d9b3823d08..04570e9d9af5c 100644 --- a/parser/hintparser.go +++ b/parser/hintparser.go @@ -1235,8 +1235,10 @@ yynewstate: } case 16: { - parser.warnUnsupportedHint(yyS[yypt-3].ident) - parser.yyVAL.hint = nil + parser.yyVAL.hint = &ast.TableOptimizerHint{ + HintName: model.NewCIStr(yyS[yypt-3].ident), + HintData: yyS[yypt-1].ident, + } } case 17: { diff --git a/parser/hintparser.y b/parser/hintparser.y index 0a29375be226d..f5c4706ff4b3c 100644 --- a/parser/hintparser.y +++ b/parser/hintparser.y @@ -279,8 +279,10 @@ TableOptimizerHintOpt: } | "RESOURCE_GROUP" '(' Identifier ')' { - parser.warnUnsupportedHint($1) - $$ = nil + $$ = &ast.TableOptimizerHint{ + HintName: model.NewCIStr($1), + HintData: $3, + } } | "QB_NAME" '(' Identifier ')' { diff --git a/parser/hintparser_test.go b/parser/hintparser_test.go index 30d8d2c1ea803..696cebd6c7be6 100644 --- a/parser/hintparser_test.go +++ b/parser/hintparser_test.go @@ -233,7 +233,7 @@ func TestParseHint(t *testing.T) { }, }, { - input: "USE_TOJA(TRUE) IGNORE_PLAN_CACHE() USE_CASCADES(TRUE) QUERY_TYPE(@qb1 OLAP) QUERY_TYPE(OLTP) NO_INDEX_MERGE()", + input: "USE_TOJA(TRUE) IGNORE_PLAN_CACHE() USE_CASCADES(TRUE) QUERY_TYPE(@qb1 OLAP) QUERY_TYPE(OLTP) NO_INDEX_MERGE() RESOURCE_GROUP(rg1)", output: []*ast.TableOptimizerHint{ { HintName: model.NewCIStr("USE_TOJA"), @@ -258,6 +258,10 @@ func TestParseHint(t *testing.T) { { HintName: model.NewCIStr("NO_INDEX_MERGE"), }, + { + HintName: model.NewCIStr("RESOURCE_GROUP"), + HintData: "rg1", + }, }, }, { diff --git a/parser/parser_test.go b/parser/parser_test.go index 1f105aeeafcd3..d110dc62b3e99 100644 --- a/parser/parser_test.go +++ b/parser/parser_test.go @@ -3874,12 +3874,12 @@ func TestOptimizerHints(t *testing.T) { require.Equal(t, "t4", hints[1].Indexes[0].L) // Test FORCE_INDEX - stmt, _, err = p.Parse("select /*+ FORCE_INDEX(T1,T2), force_index(t3,t4) */ c1, c2 from t1, t2 where t1.c1 = t2.c1", "", "") + stmt, _, err = p.Parse("select /*+ FORCE_INDEX(T1,T2), force_index(t3,t4) RESOURCE_GROUP(rg1)*/ c1, c2 from t1, t2 where t1.c1 = t2.c1", "", "") require.NoError(t, err) selectStmt = stmt[0].(*ast.SelectStmt) hints = selectStmt.TableHints - require.Len(t, hints, 2) + require.Len(t, hints, 3) require.Equal(t, "force_index", hints[0].HintName.L) require.Len(t, hints[0].Tables, 1) require.Equal(t, "t1", hints[0].Tables[0].TableName.L) @@ -3892,6 +3892,9 @@ func TestOptimizerHints(t *testing.T) { require.Len(t, hints[1].Indexes, 1) require.Equal(t, "t4", hints[1].Indexes[0].L) + require.Equal(t, "resource_group", hints[2].HintName.L) + require.Equal(t, hints[2].HintData, "rg1") + // Test IGNORE_INDEX stmt, _, err = p.Parse("select /*+ IGNORE_INDEX(T1,T2), ignore_index(t3,t4) */ c1, c2 from t1, t2 where t1.c1 = t2.c1", "", "") require.NoError(t, err) diff --git a/planner/optimize.go b/planner/optimize.go index bbd5db1ec4211..b2108a1aef4cb 100644 --- a/planner/optimize.go +++ b/planner/optimize.go @@ -162,6 +162,12 @@ func Optimize(ctx context.Context, sctx sessionctx.Context, node ast.Node, is in } } + // Override the resource group if necessary + // TODO: we didn't check the existence of the hinted resource group now to save the cost per query + if originStmtHints.HasResourceGroup { + sessVars.ResourceGroupName = originStmtHints.ResourceGroup + } + txnManger := sessiontxn.GetTxnManager(sctx) if _, isolationReadContainTiKV := sessVars.IsolationReadEngines[kv.TiKV]; isolationReadContainTiKV { var fp core.Plan @@ -616,7 +622,7 @@ func handleStmtHints(hints []*ast.TableOptimizerHint) (stmtHints stmtctx.StmtHin } hintOffs := make(map[string]int, len(hints)) var forceNthPlan *ast.TableOptimizerHint - var memoryQuotaHintCnt, useToJAHintCnt, useCascadesHintCnt, noIndexMergeHintCnt, readReplicaHintCnt, maxExecutionTimeCnt, forceNthPlanCnt, straightJoinHintCnt int + var memoryQuotaHintCnt, useToJAHintCnt, useCascadesHintCnt, noIndexMergeHintCnt, readReplicaHintCnt, maxExecutionTimeCnt, forceNthPlanCnt, straightJoinHintCnt, resourceGroupHintCnt int setVars := make(map[string]string) setVarsOffs := make([]int, 0, len(hints)) for i, hint := range hints { @@ -624,6 +630,9 @@ func handleStmtHints(hints []*ast.TableOptimizerHint) (stmtHints stmtctx.StmtHin case "memory_quota": hintOffs[hint.HintName.L] = i memoryQuotaHintCnt++ + case "resource_group": + hintOffs[hint.HintName.L] = i + resourceGroupHintCnt++ case "use_toja": hintOffs[hint.HintName.L] = i useToJAHintCnt++ @@ -747,6 +756,17 @@ func handleStmtHints(hints []*ast.TableOptimizerHint) (stmtHints stmtctx.StmtHin stmtHints.HasMaxExecutionTime = true stmtHints.MaxExecutionTime = maxExecutionTime.HintData.(uint64) } + // Handle RESOURCE_GROUP + if resourceGroupHintCnt != 0 { + resourceGroup := hints[hintOffs["resource_group"]] + if resourceGroupHintCnt > 1 { + warn := errors.Errorf("RESOURCE_GROUP() is defined more than once, only the last definition takes effect: RESOURCE_GROUP(%v)", resourceGroup.HintData.(string)) + warns = append(warns, warn) + } + stmtHints.HasResourceGroup = true + // TODO: check the existence of the resource group + stmtHints.ResourceGroup = resourceGroup.HintData.(string) + } // Handle NTH_PLAN if forceNthPlanCnt != 0 { if forceNthPlanCnt > 1 { diff --git a/session/session.go b/session/session.go index 1e7ec13cab233..04412e7da05d3 100644 --- a/session/session.go +++ b/session/session.go @@ -2186,6 +2186,9 @@ func (s *session) ExecuteStmt(ctx context.Context, stmtNode ast.StmtNode) (sqlex stmtLabel := ast.GetStmtLabel(stmtNode) s.setRequestSource(ctx, stmtLabel, stmtNode) + // Backup the original resource group name since sql hint might change it during optimization + originalResourceGroup := s.GetSessionVars().ResourceGroupName + // Transform abstract syntax tree to a physical plan(stored in executor.ExecStmt). compiler := executor.Compiler{Ctx: s} stmt, err := compiler.Compile(ctx, stmtNode) @@ -2236,6 +2239,9 @@ func (s *session) ExecuteStmt(ctx context.Context, stmtNode ast.StmtNode) (sqlex metrics.ResourceGroupQueryTotalCounter.WithLabelValues(resourceGroupName).Inc() } + // Restore the resource group for the session + s.GetSessionVars().ResourceGroupName = originalResourceGroup + if err != nil { if !errIsNoisy(err) { logutil.Logger(ctx).Warn("run statement failed", diff --git a/sessionctx/stmtctx/stmtctx.go b/sessionctx/stmtctx/stmtctx.go index 01ead10e580fc..25ae64e91c6c2 100644 --- a/sessionctx/stmtctx/stmtctx.go +++ b/sessionctx/stmtctx/stmtctx.go @@ -406,7 +406,8 @@ type StmtHints struct { EnableCascadesPlanner bool // ForceNthPlan indicates the PlanCounterTp number for finding physical plan. // -1 for disable. - ForceNthPlan int64 + ForceNthPlan int64 + ResourceGroup string // Hint flags HasAllowInSubqToJoinAndAggHint bool @@ -414,6 +415,7 @@ type StmtHints struct { HasReplicaReadHint bool HasMaxExecutionTime bool HasEnableCascadesPlannerHint bool + HasResourceGroup bool SetVars map[string]string // the original table hints From 281b4faaddf4f6bc9c449882b8de04037787903b Mon Sep 17 00:00:00 2001 From: BornChanger Date: Wed, 22 Feb 2023 20:19:20 +0800 Subject: [PATCH 02/12] *: refine case Signed-off-by: BornChanger --- ddl/resource_group_test.go | 3 ++- planner/optimize.go | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ddl/resource_group_test.go b/ddl/resource_group_test.go index 40f4266a60e24..46d51f4f6dd49 100644 --- a/ddl/resource_group_test.go +++ b/ddl/resource_group_test.go @@ -189,7 +189,8 @@ func TestResourceGroupHint(t *testing.T) { tk.MustExec("create resource group rg1 ru_per_sec=1000") tk.MustQuery("select /*+ resource_group(default) */ * from t1") tk.MustQuery("select /*+ resource_group(rg1) */ * from t1") - tk.MustQuery("select /*+ resource_group(rg2) */ * from t1") + tk.MustQuery("select /*+ resource_group(rg1) resource_group(default) */ * from t1") + tk.MustQuery("show warnings").Check(testkit.Rows("Warning 1105 RESOURCE_GROUP() is defined more than once, only the last definition takes effect: RESOURCE_GROUP(default)")) tk.MustQuery("select /*+ resource_group(rg1) */ * from information_schema.processlist").Check(testkit.Rows("1 test Sleep 0 autocommit select /*+ resource_group(rg1) */ * from information_schema.processlist 4b5e7cdd5d3ed84d6c1a6d56403a3d512554b534313caf296268abdec1c9ea99 0 0 rg1")) tk.MustQuery("select * from information_schema.processlist").Check(testkit.Rows("1 test Sleep 0 autocommit select * from information_schema.processlist 4b5e7cdd5d3ed84d6c1a6d56403a3d512554b534313caf296268abdec1c9ea99 0 0 ")) diff --git a/planner/optimize.go b/planner/optimize.go index b2108a1aef4cb..b7b51f3d2d204 100644 --- a/planner/optimize.go +++ b/planner/optimize.go @@ -764,7 +764,6 @@ func handleStmtHints(hints []*ast.TableOptimizerHint) (stmtHints stmtctx.StmtHin warns = append(warns, warn) } stmtHints.HasResourceGroup = true - // TODO: check the existence of the resource group stmtHints.ResourceGroup = resourceGroup.HintData.(string) } // Handle NTH_PLAN From 3b025482c34baecabbf0d4224187b76cc49755ac Mon Sep 17 00:00:00 2001 From: BornChanger Date: Wed, 22 Feb 2023 20:48:02 +0800 Subject: [PATCH 03/12] *: make case stable Signed-off-by: BornChanger --- ddl/resource_group_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ddl/resource_group_test.go b/ddl/resource_group_test.go index 46d51f4f6dd49..90b9495ea59e1 100644 --- a/ddl/resource_group_test.go +++ b/ddl/resource_group_test.go @@ -191,7 +191,7 @@ func TestResourceGroupHint(t *testing.T) { tk.MustQuery("select /*+ resource_group(rg1) */ * from t1") tk.MustQuery("select /*+ resource_group(rg1) resource_group(default) */ * from t1") tk.MustQuery("show warnings").Check(testkit.Rows("Warning 1105 RESOURCE_GROUP() is defined more than once, only the last definition takes effect: RESOURCE_GROUP(default)")) - tk.MustQuery("select /*+ resource_group(rg1) */ * from information_schema.processlist").Check(testkit.Rows("1 test Sleep 0 autocommit select /*+ resource_group(rg1) */ * from information_schema.processlist 4b5e7cdd5d3ed84d6c1a6d56403a3d512554b534313caf296268abdec1c9ea99 0 0 rg1")) - tk.MustQuery("select * from information_schema.processlist").Check(testkit.Rows("1 test Sleep 0 autocommit select * from information_schema.processlist 4b5e7cdd5d3ed84d6c1a6d56403a3d512554b534313caf296268abdec1c9ea99 0 0 ")) + tk.MustQuery("select /*+ resource_group(rg1) */ DB, RESOURCE_GROUP from information_schema.processlist").Check(testkit.Rows("test rg1")) + tk.MustQuery("select DB, RESOURCE_GROUP from information_schema.processlist").Check(testkit.Rows("test ")) } From fcc0cf330f9a69cbd5ffe1132862b35855e36801 Mon Sep 17 00:00:00 2001 From: BornChanger Date: Wed, 22 Feb 2023 23:23:38 +0800 Subject: [PATCH 04/12] *: code format Signed-off-by: BornChanger --- ddl/resource_group_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/ddl/resource_group_test.go b/ddl/resource_group_test.go index 90b9495ea59e1..29ae56bcda820 100644 --- a/ddl/resource_group_test.go +++ b/ddl/resource_group_test.go @@ -193,5 +193,4 @@ func TestResourceGroupHint(t *testing.T) { tk.MustQuery("show warnings").Check(testkit.Rows("Warning 1105 RESOURCE_GROUP() is defined more than once, only the last definition takes effect: RESOURCE_GROUP(default)")) tk.MustQuery("select /*+ resource_group(rg1) */ DB, RESOURCE_GROUP from information_schema.processlist").Check(testkit.Rows("test rg1")) tk.MustQuery("select DB, RESOURCE_GROUP from information_schema.processlist").Check(testkit.Rows("test ")) - } From ad3e9a82b67e6a12e0fbd2a3f916d32c89d3bad5 Mon Sep 17 00:00:00 2001 From: BornChanger Date: Mon, 27 Feb 2023 15:40:58 +0800 Subject: [PATCH 05/12] *: relocate the cases Signed-off-by: BornChanger --- ddl/BUILD.bazel | 1 - ddl/resourcegroup/BUILD.bazel | 15 +- ddl/resourcegroup/group_test.go | 143 ------------------ ddl/resourcegrouptest/BUILD.bazel | 18 +++ .../resource_group_test.go | 126 ++++++++++++++- 5 files changed, 143 insertions(+), 160 deletions(-) delete mode 100644 ddl/resourcegroup/group_test.go create mode 100644 ddl/resourcegrouptest/BUILD.bazel rename ddl/{ => resourcegrouptest}/resource_group_test.go (74%) diff --git a/ddl/BUILD.bazel b/ddl/BUILD.bazel index 2fbe94b2fcd16..04c70a7671542 100644 --- a/ddl/BUILD.bazel +++ b/ddl/BUILD.bazel @@ -198,7 +198,6 @@ go_test( "primary_key_handle_test.go", "reorg_partition_test.go", "repair_table_test.go", - "resource_group_test.go", "restart_test.go", "rollingback_test.go", "schema_test.go", diff --git a/ddl/resourcegroup/BUILD.bazel b/ddl/resourcegroup/BUILD.bazel index ae19c6f654e86..47f9367b626cb 100644 --- a/ddl/resourcegroup/BUILD.bazel +++ b/ddl/resourcegroup/BUILD.bazel @@ -1,4 +1,4 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") +load("@io_bazel_rules_go//go:def.bzl", "go_library") go_library( name = "resourcegroup", @@ -14,16 +14,3 @@ go_library( "@com_github_pingcap_kvproto//pkg/resource_manager", ], ) - -go_test( - name = "resourcegroup_test", - timeout = "short", - srcs = ["group_test.go"], - embed = [":resourcegroup"], - flaky = True, - deps = [ - "//parser/model", - "@com_github_pingcap_kvproto//pkg/resource_manager", - "@com_github_stretchr_testify//require", - ], -) diff --git a/ddl/resourcegroup/group_test.go b/ddl/resourcegroup/group_test.go deleted file mode 100644 index 7d79f00aa6184..0000000000000 --- a/ddl/resourcegroup/group_test.go +++ /dev/null @@ -1,143 +0,0 @@ -// Copyright 2022 PingCAP, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package resourcegroup - -import ( - "fmt" - "testing" - - rmpb "github.com/pingcap/kvproto/pkg/resource_manager" - "github.com/pingcap/tidb/parser/model" - "github.com/stretchr/testify/require" -) - -func TestNewResourceGroupFromOptions(t *testing.T) { - type TestCase struct { - name string - groupName string - input *model.ResourceGroupSettings - output *rmpb.ResourceGroup - err error - } - var tests []TestCase - groupName := "test" - tests = append(tests, TestCase{ - name: "empty 1", - input: &model.ResourceGroupSettings{}, - err: ErrUnknownResourceGroupMode, - }) - - tests = append(tests, TestCase{ - name: "empty 2", - input: nil, - err: ErrInvalidGroupSettings, - }) - - tests = append(tests, TestCase{ - name: "normal case: ru case 1", - input: &model.ResourceGroupSettings{ - RURate: 2000, - }, - output: &rmpb.ResourceGroup{ - Name: groupName, - Mode: rmpb.GroupMode_RUMode, - RUSettings: &rmpb.GroupRequestUnitSettings{ - RU: &rmpb.TokenBucket{Settings: &rmpb.TokenLimitSettings{FillRate: 2000}}, - }, - }, - }) - - tests = append(tests, TestCase{ - name: "normal case: ru case 2", - input: &model.ResourceGroupSettings{ - RURate: 5000, - }, - output: &rmpb.ResourceGroup{ - Name: groupName, - Mode: rmpb.GroupMode_RUMode, - RUSettings: &rmpb.GroupRequestUnitSettings{ - RU: &rmpb.TokenBucket{Settings: &rmpb.TokenLimitSettings{FillRate: 5000}}, - }, - }, - }) - - tests = append(tests, TestCase{ - name: "error case: native case 1", - input: &model.ResourceGroupSettings{ - CPULimiter: "8", - IOReadBandwidth: "3000MB/s", - IOWriteBandwidth: "3000Mi", - }, - err: ErrUnknownResourceGroupMode, - }) - - tests = append(tests, TestCase{ - name: "error case: native case 2", - input: &model.ResourceGroupSettings{ - CPULimiter: "8c", - IOReadBandwidth: "3000Mi", - IOWriteBandwidth: "3000Mi", - }, - err: ErrUnknownResourceGroupMode, - }) - - tests = append(tests, TestCase{ - name: "error case: native case 3", - input: &model.ResourceGroupSettings{ - CPULimiter: "8", - IOReadBandwidth: "3000G", - IOWriteBandwidth: "3000MB", - }, - err: ErrUnknownResourceGroupMode, - }) - - tests = append(tests, TestCase{ - name: "error case: duplicated mode", - input: &model.ResourceGroupSettings{ - CPULimiter: "8", - IOReadBandwidth: "3000Mi", - IOWriteBandwidth: "3000Mi", - RURate: 1000, - }, - err: ErrInvalidResourceGroupDuplicatedMode, - }) - - tests = append(tests, TestCase{ - name: "error case: duplicated mode", - groupName: "test_group_too_looooooooooooooooooooooooooooooooooooooooooooooooong", - input: &model.ResourceGroupSettings{ - CPULimiter: "8", - IOReadBandwidth: "3000Mi", - IOWriteBandwidth: "3000Mi", - RURate: 1000, - }, - err: ErrTooLongResourceGroupName, - }) - - for _, test := range tests { - name := groupName - if len(test.groupName) > 0 { - name = test.groupName - } - group, err := NewGroupFromOptions(name, test.input) - comment := fmt.Sprintf("[%s]\nerr1 %s\nerr2 %s", test.name, err, test.err) - if test.err != nil { - require.ErrorIs(t, err, test.err, comment) - } else { - require.NoError(t, err, comment) - require.Equal(t, test.output, group) - } - } -} diff --git a/ddl/resourcegrouptest/BUILD.bazel b/ddl/resourcegrouptest/BUILD.bazel new file mode 100644 index 0000000000000..0811697a8d950 --- /dev/null +++ b/ddl/resourcegrouptest/BUILD.bazel @@ -0,0 +1,18 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_test") + +go_test( + name = "resourcegrouptest_test", + srcs = ["resource_group_test.go"], + deps = [ + "//ddl/internal/callback", + "//ddl/resourcegroup", + "//domain", + "//domain/infosync", + "//errno", + "//parser/model", + "//sessionctx", + "//testkit", + "@com_github_pingcap_kvproto//pkg/resource_manager", + "@com_github_stretchr_testify//require", + ], +) diff --git a/ddl/resource_group_test.go b/ddl/resourcegrouptest/resource_group_test.go similarity index 74% rename from ddl/resource_group_test.go rename to ddl/resourcegrouptest/resource_group_test.go index 29ae56bcda820..41ad5dae47058 100644 --- a/ddl/resource_group_test.go +++ b/ddl/resourcegrouptest/resource_group_test.go @@ -12,12 +12,15 @@ // See the License for the specific language governing permissions and // limitations under the License. -package ddl_test +package resourcegrouptest_test import ( "context" + rmpb "github.com/pingcap/kvproto/pkg/resource_manager" + "github.com/pingcap/tidb/ddl/resourcegroup" "testing" + "fmt" "github.com/pingcap/tidb/ddl/internal/callback" "github.com/pingcap/tidb/domain" "github.com/pingcap/tidb/domain/infosync" @@ -178,7 +181,7 @@ func testResourceGroupNameFromIS(t *testing.T, ctx sessionctx.Context, name stri } func TestResourceGroupHint(t *testing.T) { - store, _ := testkit.CreateMockStoreAndDomain(t) + store := testkit.CreateMockStore(t) tk := testkit.NewTestKit(t, store) tk.MustExec("use test") @@ -194,3 +197,122 @@ func TestResourceGroupHint(t *testing.T) { tk.MustQuery("select /*+ resource_group(rg1) */ DB, RESOURCE_GROUP from information_schema.processlist").Check(testkit.Rows("test rg1")) tk.MustQuery("select DB, RESOURCE_GROUP from information_schema.processlist").Check(testkit.Rows("test ")) } + +func TestNewResourceGroupFromOptions(t *testing.T) { + type TestCase struct { + name string + groupName string + input *model.ResourceGroupSettings + output *rmpb.ResourceGroup + err error + } + var tests []TestCase + groupName := "test" + tests = append(tests, TestCase{ + name: "empty 1", + input: &model.ResourceGroupSettings{}, + err: resourcegroup.ErrUnknownResourceGroupMode, + }) + + tests = append(tests, TestCase{ + name: "empty 2", + input: nil, + err: resourcegroup.ErrInvalidGroupSettings, + }) + + tests = append(tests, TestCase{ + name: "normal case: ru case 1", + input: &model.ResourceGroupSettings{ + RURate: 2000, + }, + output: &rmpb.ResourceGroup{ + Name: groupName, + Mode: rmpb.GroupMode_RUMode, + RUSettings: &rmpb.GroupRequestUnitSettings{ + RU: &rmpb.TokenBucket{Settings: &rmpb.TokenLimitSettings{FillRate: 2000}}, + }, + }, + }) + + tests = append(tests, TestCase{ + name: "normal case: ru case 2", + input: &model.ResourceGroupSettings{ + RURate: 5000, + }, + output: &rmpb.ResourceGroup{ + Name: groupName, + Mode: rmpb.GroupMode_RUMode, + RUSettings: &rmpb.GroupRequestUnitSettings{ + RU: &rmpb.TokenBucket{Settings: &rmpb.TokenLimitSettings{FillRate: 5000}}, + }, + }, + }) + + tests = append(tests, TestCase{ + name: "error case: native case 1", + input: &model.ResourceGroupSettings{ + CPULimiter: "8", + IOReadBandwidth: "3000MB/s", + IOWriteBandwidth: "3000Mi", + }, + err: resourcegroup.ErrUnknownResourceGroupMode, + }) + + tests = append(tests, TestCase{ + name: "error case: native case 2", + input: &model.ResourceGroupSettings{ + CPULimiter: "8c", + IOReadBandwidth: "3000Mi", + IOWriteBandwidth: "3000Mi", + }, + err: resourcegroup.ErrUnknownResourceGroupMode, + }) + + tests = append(tests, TestCase{ + name: "error case: native case 3", + input: &model.ResourceGroupSettings{ + CPULimiter: "8", + IOReadBandwidth: "3000G", + IOWriteBandwidth: "3000MB", + }, + err: resourcegroup.ErrUnknownResourceGroupMode, + }) + + tests = append(tests, TestCase{ + name: "error case: duplicated mode", + input: &model.ResourceGroupSettings{ + CPULimiter: "8", + IOReadBandwidth: "3000Mi", + IOWriteBandwidth: "3000Mi", + RURate: 1000, + }, + err: resourcegroup.ErrInvalidResourceGroupDuplicatedMode, + }) + + tests = append(tests, TestCase{ + name: "error case: duplicated mode", + groupName: "test_group_too_looooooooooooooooooooooooooooooooooooooooooooooooong", + input: &model.ResourceGroupSettings{ + CPULimiter: "8", + IOReadBandwidth: "3000Mi", + IOWriteBandwidth: "3000Mi", + RURate: 1000, + }, + err: resourcegroup.ErrTooLongResourceGroupName, + }) + + for _, test := range tests { + name := groupName + if len(test.groupName) > 0 { + name = test.groupName + } + group, err := resourcegroup.NewGroupFromOptions(name, test.input) + comment := fmt.Sprintf("[%s]\nerr1 %s\nerr2 %s", test.name, err, test.err) + if test.err != nil { + require.ErrorIs(t, err, test.err, comment) + } else { + require.NoError(t, err, comment) + require.Equal(t, test.output, group) + } + } +} From 5f66837ecc719d6fd50e6388a666b4f504b9fb01 Mon Sep 17 00:00:00 2001 From: BornChanger Date: Mon, 27 Feb 2023 15:46:15 +0800 Subject: [PATCH 06/12] *: fix bazel attribute Signed-off-by: BornChanger --- ddl/resourcegrouptest/BUILD.bazel | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ddl/resourcegrouptest/BUILD.bazel b/ddl/resourcegrouptest/BUILD.bazel index 0811697a8d950..4e4fff0c6c739 100644 --- a/ddl/resourcegrouptest/BUILD.bazel +++ b/ddl/resourcegrouptest/BUILD.bazel @@ -3,6 +3,8 @@ load("@io_bazel_rules_go//go:def.bzl", "go_test") go_test( name = "resourcegrouptest_test", srcs = ["resource_group_test.go"], + timeout = "short", + flaky = True, deps = [ "//ddl/internal/callback", "//ddl/resourcegroup", From 12df0b980cd20737f9a0f5f6ea69cb47411a66f8 Mon Sep 17 00:00:00 2001 From: BornChanger Date: Mon, 27 Feb 2023 15:55:32 +0800 Subject: [PATCH 07/12] *: fix bazel Signed-off-by: BornChanger --- ddl/resourcegrouptest/BUILD.bazel | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddl/resourcegrouptest/BUILD.bazel b/ddl/resourcegrouptest/BUILD.bazel index 4e4fff0c6c739..64cab4df3d65f 100644 --- a/ddl/resourcegrouptest/BUILD.bazel +++ b/ddl/resourcegrouptest/BUILD.bazel @@ -2,8 +2,8 @@ load("@io_bazel_rules_go//go:def.bzl", "go_test") go_test( name = "resourcegrouptest_test", - srcs = ["resource_group_test.go"], timeout = "short", + srcs = ["resource_group_test.go"], flaky = True, deps = [ "//ddl/internal/callback", From 30d36e151571a467acd5f61dc023ac3e6de414d6 Mon Sep 17 00:00:00 2001 From: BornChanger Date: Tue, 28 Feb 2023 10:38:54 +0800 Subject: [PATCH 08/12] *: code fmt Signed-off-by: BornChanger --- ddl/resourcegrouptest/resource_group_test.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/ddl/resourcegrouptest/resource_group_test.go b/ddl/resourcegrouptest/resource_group_test.go index 41ad5dae47058..da34e582713b8 100644 --- a/ddl/resourcegrouptest/resource_group_test.go +++ b/ddl/resourcegrouptest/resource_group_test.go @@ -16,12 +16,10 @@ package resourcegrouptest_test import ( "context" - rmpb "github.com/pingcap/kvproto/pkg/resource_manager" - "github.com/pingcap/tidb/ddl/resourcegroup" - "testing" - "fmt" + rmpb "github.com/pingcap/kvproto/pkg/resource_manager" "github.com/pingcap/tidb/ddl/internal/callback" + "github.com/pingcap/tidb/ddl/resourcegroup" "github.com/pingcap/tidb/domain" "github.com/pingcap/tidb/domain/infosync" mysql "github.com/pingcap/tidb/errno" @@ -29,6 +27,7 @@ import ( "github.com/pingcap/tidb/sessionctx" "github.com/pingcap/tidb/testkit" "github.com/stretchr/testify/require" + "testing" ) func TestResourceGroupBasic(t *testing.T) { From ffcc68cfea6a8c416fe52c49f7ffa721dc0b59f9 Mon Sep 17 00:00:00 2001 From: BornChanger Date: Tue, 28 Feb 2023 15:00:26 +0800 Subject: [PATCH 09/12] *: code format Signed-off-by: BornChanger --- ddl/resourcegrouptest/resource_group_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ddl/resourcegrouptest/resource_group_test.go b/ddl/resourcegrouptest/resource_group_test.go index da34e582713b8..a186d785d11f1 100644 --- a/ddl/resourcegrouptest/resource_group_test.go +++ b/ddl/resourcegrouptest/resource_group_test.go @@ -17,6 +17,8 @@ package resourcegrouptest_test import ( "context" "fmt" + "testing" + rmpb "github.com/pingcap/kvproto/pkg/resource_manager" "github.com/pingcap/tidb/ddl/internal/callback" "github.com/pingcap/tidb/ddl/resourcegroup" @@ -27,7 +29,6 @@ import ( "github.com/pingcap/tidb/sessionctx" "github.com/pingcap/tidb/testkit" "github.com/stretchr/testify/require" - "testing" ) func TestResourceGroupBasic(t *testing.T) { From fea34d0f115840b82c72bd9ffec9c3d080ae5a1e Mon Sep 17 00:00:00 2001 From: BornChanger Date: Fri, 3 Mar 2023 14:55:12 +0800 Subject: [PATCH 10/12] *: address comments Signed-off-by: BornChanger --- session/session.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/session/session.go b/session/session.go index 04412e7da05d3..4417800240c24 100644 --- a/session/session.go +++ b/session/session.go @@ -2189,6 +2189,11 @@ func (s *session) ExecuteStmt(ctx context.Context, stmtNode ast.StmtNode) (sqlex // Backup the original resource group name since sql hint might change it during optimization originalResourceGroup := s.GetSessionVars().ResourceGroupName + defer func() { + // Restore the resource group for the session + s.GetSessionVars().ResourceGroupName = originalResourceGroup + }() + // Transform abstract syntax tree to a physical plan(stored in executor.ExecStmt). compiler := executor.Compiler{Ctx: s} stmt, err := compiler.Compile(ctx, stmtNode) @@ -2239,9 +2244,6 @@ func (s *session) ExecuteStmt(ctx context.Context, stmtNode ast.StmtNode) (sqlex metrics.ResourceGroupQueryTotalCounter.WithLabelValues(resourceGroupName).Inc() } - // Restore the resource group for the session - s.GetSessionVars().ResourceGroupName = originalResourceGroup - if err != nil { if !errIsNoisy(err) { logutil.Logger(ctx).Warn("run statement failed", From 3dc928ac16b66865e1543bf381a1ca7f0d85a2d8 Mon Sep 17 00:00:00 2001 From: BornChanger Date: Fri, 3 Mar 2023 19:14:39 +0800 Subject: [PATCH 11/12] *: address comments for 2nd round Signed-off-by: BornChanger --- ddl/resourcegrouptest/resource_group_test.go | 3 +++ planner/optimize.go | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/ddl/resourcegrouptest/resource_group_test.go b/ddl/resourcegrouptest/resource_group_test.go index a186d785d11f1..fcd11aa2a7b3a 100644 --- a/ddl/resourcegrouptest/resource_group_test.go +++ b/ddl/resourcegrouptest/resource_group_test.go @@ -196,6 +196,9 @@ func TestResourceGroupHint(t *testing.T) { tk.MustQuery("show warnings").Check(testkit.Rows("Warning 1105 RESOURCE_GROUP() is defined more than once, only the last definition takes effect: RESOURCE_GROUP(default)")) tk.MustQuery("select /*+ resource_group(rg1) */ DB, RESOURCE_GROUP from information_schema.processlist").Check(testkit.Rows("test rg1")) tk.MustQuery("select DB, RESOURCE_GROUP from information_schema.processlist").Check(testkit.Rows("test ")) + tk.MustExec("set global tidb_enable_resource_control='off'") + tk.MustQuery("select /*+ resource_group(rg1) */ DB, RESOURCE_GROUP from information_schema.processlist").Check(testkit.Rows("test ")) + tk.MustQuery("show warnings").Check(testkit.Rows("Warning 8250 Resource control feature is disabled. Run `SET GLOBAL tidb_enable_resource_control='on'` to enable the feature")) } func TestNewResourceGroupFromOptions(t *testing.T) { diff --git a/planner/optimize.go b/planner/optimize.go index b7b51f3d2d204..a2b62ca05d6ee 100644 --- a/planner/optimize.go +++ b/planner/optimize.go @@ -164,8 +164,11 @@ func Optimize(ctx context.Context, sctx sessionctx.Context, node ast.Node, is in // Override the resource group if necessary // TODO: we didn't check the existence of the hinted resource group now to save the cost per query - if originStmtHints.HasResourceGroup { + if originStmtHints.HasResourceGroup && variable.EnableResourceControl.Load() { sessVars.ResourceGroupName = originStmtHints.ResourceGroup + } else if originStmtHints.HasResourceGroup { + err := infoschema.ErrResourceGroupSupportDisabled + sessVars.StmtCtx.AppendWarning(err) } txnManger := sessiontxn.GetTxnManager(sctx) From be210d89a881912dd6f729d56e69eebb86c53904 Mon Sep 17 00:00:00 2001 From: BornChanger Date: Mon, 6 Mar 2023 12:46:46 +0800 Subject: [PATCH 12/12] *: polish check logic Signed-off-by: BornChanger --- planner/optimize.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/planner/optimize.go b/planner/optimize.go index a2b62ca05d6ee..33c672206a2ef 100644 --- a/planner/optimize.go +++ b/planner/optimize.go @@ -164,11 +164,13 @@ func Optimize(ctx context.Context, sctx sessionctx.Context, node ast.Node, is in // Override the resource group if necessary // TODO: we didn't check the existence of the hinted resource group now to save the cost per query - if originStmtHints.HasResourceGroup && variable.EnableResourceControl.Load() { - sessVars.ResourceGroupName = originStmtHints.ResourceGroup - } else if originStmtHints.HasResourceGroup { - err := infoschema.ErrResourceGroupSupportDisabled - sessVars.StmtCtx.AppendWarning(err) + if originStmtHints.HasResourceGroup { + if variable.EnableResourceControl.Load() { + sessVars.ResourceGroupName = originStmtHints.ResourceGroup + } else { + err := infoschema.ErrResourceGroupSupportDisabled + sessVars.StmtCtx.AppendWarning(err) + } } txnManger := sessiontxn.GetTxnManager(sctx)