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

*: refine mysql.tidb_mdl_view #48728

Merged
merged 10 commits into from
Jan 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/infoschema/test/clustertablestest/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ go_test(
"//pkg/parser/mysql",
"//pkg/parser/terror",
"//pkg/planner/core",
"//pkg/privilege/privileges",
"//pkg/server",
"//pkg/session",
"//pkg/session/txninfo",
Expand Down
22 changes: 18 additions & 4 deletions pkg/infoschema/test/clustertablestest/cluster_tables_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import (
"github.com/pingcap/tidb/pkg/parser"
"github.com/pingcap/tidb/pkg/parser/auth"
"github.com/pingcap/tidb/pkg/parser/mysql"
"github.com/pingcap/tidb/pkg/privilege/privileges"
"github.com/pingcap/tidb/pkg/server"
"github.com/pingcap/tidb/pkg/store/mockstore"
"github.com/pingcap/tidb/pkg/store/mockstore/mockstorage"
Expand Down Expand Up @@ -833,14 +834,21 @@ func (s *clusterTablesSuite) newTestKitWithRoot(t *testing.T) *testkit.TestKit {
func TestMDLView(t *testing.T) {
testCases := []struct {
name string
createTable string
createTable []string
ddl string
queryInTxn []string
sqlDigest string
}{
{"add column", "create table t(a int)", "alter table test.t add column b int", []string{"select 1", "select * from t"}, "[\"begin\",\"select ?\",\"select * from `t`\"]"},
{"change column in 1 step", "create table t(a int)", "alter table test.t change column a b int", []string{"select 1", "select * from t"}, "[\"begin\",\"select ?\",\"select * from `t`\"]"},
{"add column", []string{"create table t(a int)"}, "alter table test.t add column b int", []string{"select 1", "select * from t"}, "[\"begin\",\"select ?\",\"select * from `t`\"]"},
{"change column in 1 step", []string{"create table t(a int)"}, "alter table test.t change column a b int", []string{"select 1", "select * from t"}, "[\"begin\",\"select ?\",\"select * from `t`\"]"},
{"rename tables", []string{"create table t(a int)", "create table t1(a int)"}, "rename table test.t to test.t2, test.t1 to test.t3", []string{"select 1", "select * from t"}, "[\"begin\",\"select ?\",\"select * from `t`\"]"},
{"err don't show rollbackdone ddl", []string{"create table t(a int)", "insert into t values (1);", "insert into t values (1);", "alter table t add unique idx(id);"}, "alter table test.t add column b int", []string{"select 1", "select * from t"}, "[\"begin\",\"select ?\",\"select * from `t`\"]"},
}
save := privileges.SkipWithGrant
privileges.SkipWithGrant = true
defer func() {
privileges.SkipWithGrant = save
}()
for _, c := range testCases {
t.Run(c.name, func(t *testing.T) {
// setup suite
Expand All @@ -855,7 +863,13 @@ func TestMDLView(t *testing.T) {
tk3 := s.newTestKitWithRoot(t)
tk.MustExec("use test")
tk.MustExec("set global tidb_enable_metadata_lock=1")
tk.MustExec(c.createTable)
for _, cr := range c.createTable {
if strings.Contains(c.name, "err") {
_, _ = tk.Exec(cr)
Copy link
Contributor

Choose a reason for hiding this comment

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

check it does return err?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not all the SQLs return an error, I don't think it's necessary to check the error since it is just a preparation.

} else {
tk.MustExec(cr)
}
}

tk.MustExec("begin")
for _, q := range c.queryInTxn {
Expand Down
36 changes: 24 additions & 12 deletions pkg/session/bootstrap.go
Original file line number Diff line number Diff line change
Expand Up @@ -460,19 +460,18 @@ const (
);`
// CreateMDLView is a view about metadata locks.
CreateMDLView = `CREATE OR REPLACE VIEW mysql.tidb_mdl_view as (
SELECT job_id,
db_name,
table_name,
query,
SELECT tidb_mdl_info.job_id,
JSON_UNQUOTE(JSON_EXTRACT(cast(cast(job_meta as char) as json), "$.schema_name")) as db_name,
JSON_UNQUOTE(JSON_EXTRACT(cast(cast(job_meta as char) as json), "$.table_name")) as table_name,
JSON_UNQUOTE(JSON_EXTRACT(cast(cast(job_meta as char) as json), "$.query")) as query,
session_id,
txnstart,
cluster_tidb_trx.start_time,
tidb_decode_sql_digests(all_sql_digests, 4096) AS SQL_DIGESTS
FROM information_schema.ddl_jobs,
information_schema.cluster_tidb_trx,
information_schema.cluster_processlist
WHERE (ddl_jobs.state != 'synced' and ddl_jobs.state != 'cancelled')
AND Find_in_set(ddl_jobs.table_id, cluster_tidb_trx.related_table_ids)
AND cluster_tidb_trx.session_id = cluster_processlist.id
FROM mysql.tidb_ddl_job,
Copy link
Contributor

Choose a reason for hiding this comment

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

TiDB versions less than 6.2 do not have this table. Does this have compatibility issues?

Copy link
Member Author

Choose a reason for hiding this comment

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

After testing, the old owner doesn't check if the table exists or not. So there it no compatibility issues for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Maybe we can create an issue.

mysql.tidb_mdl_info,
information_schema.cluster_tidb_trx
WHERE tidb_ddl_job.job_id=tidb_mdl_info.job_id
AND CONCAT(',', tidb_mdl_info.table_ids, ',') REGEXP CONCAT(',', REPLACE(cluster_tidb_trx.related_table_ids, ',', '|'), ',') != 0
);`

// CreatePlanReplayerStatusTable is a table about plan replayer status
Expand Down Expand Up @@ -1044,6 +1043,7 @@ const (
// version 179
// enlarge `VARIABLE_VALUE` of `mysql.global_variables` from `varchar(1024)` to `varchar(16383)`.
version179 = 179

// version 180
// add priority/create_time/end_time to `mysql.tidb_global_task`/`mysql.tidb_global_task_history`
// add concurrency/create_time/end_time/digest to `mysql.tidb_background_subtask`/`mysql.tidb_background_subtask_history`
Expand All @@ -1059,11 +1059,15 @@ const (
// add new system table `mysql.request_unit_by_group`, which is used for
// historical RU consumption by resource group per day.
version182 = 182

// version 183
// replace `mysql.tidb_mdl_view` table
version183 = 183
)

// currentBootstrapVersion is defined as a variable, so we can modify its value for testing.
// please make sure this is the largest version
var currentBootstrapVersion int64 = version182
var currentBootstrapVersion int64 = version183

// DDL owner key's expired time is ManagerSessionTTL seconds, we should wait the time and give more time to have a chance to finish it.
var internalSQLTimeout = owner.ManagerSessionTTL + 15
Expand Down Expand Up @@ -1221,6 +1225,7 @@ var (
upgradeToVer180,
upgradeToVer181,
upgradeToVer182,
upgradeToVer183,
}
)

Expand Down Expand Up @@ -2977,6 +2982,13 @@ func upgradeToVer182(s sessiontypes.Session, ver int64) {
doReentrantDDL(s, CreateRequestUnitByGroupTable)
}

func upgradeToVer183(s sessiontypes.Session, ver int64) {
if ver >= version183 {
return
}
doReentrantDDL(s, CreateMDLView)
}

func writeOOMAction(s sessiontypes.Session) {
comment := "oom-action is `log` by default in v3.0.x, `cancel` by default in v4.0.11+"
mustExecute(s, `INSERT HIGH_PRIORITY INTO %n.%n VALUES (%?, %?, %?) ON DUPLICATE KEY UPDATE VARIABLE_VALUE= %?`,
Expand Down
2 changes: 1 addition & 1 deletion pkg/session/bootstraptest/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_test")

go_test(
name = "bootstraptest_test",
timeout = "short",
timeout = "moderate",
srcs = [
"bootstrap_upgrade_test.go", #keep
"main_test.go",
Expand Down
5 changes: 5 additions & 0 deletions pkg/session/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -3707,6 +3707,11 @@ func (s *session) loadCommonGlobalVariablesIfNeeded() error {
}
if s.Value(sessionctx.Initing) != nil {
// When running bootstrap or upgrade, we should not access global storage.
// But we need to init max_allowed_packet to use concat function during bootstrap or upgrade.
err := vars.SetSystemVar(variable.MaxAllowedPacket, strconv.FormatUint(variable.DefMaxAllowedPacket, 10))
if err != nil {
logutil.BgLogger().Error("set system variable max_allowed_packet error", zap.Error(err))
Copy link
Contributor

Choose a reason for hiding this comment

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

should we return err if it's required?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not always required, only when we need to create or replace this view. Do you think we still need to return an error?

}
return nil
}

Expand Down