Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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/ddl/schematracker/checker.go
Original file line number Diff line number Diff line change
Expand Up @@ -575,6 +575,7 @@ func (d *Checker) DoDDLJobWrapper(ctx sessionctx.Context, jobW *ddl.JobWrapper)

type storageAndMore interface {
kv.Storage
kv.StorageWithPD
kv.EtcdBackend
helper.Storage
}
Expand Down
37 changes: 35 additions & 2 deletions pkg/session/bootstrap.go
Original file line number Diff line number Diff line change
Expand Up @@ -854,6 +854,8 @@ const (
tidbDefOOMAction = "default_oom_action"
// The variable name in mysql.tidb table and it records the current DDLTableVersion
tidbDDLTableVersion = "ddl_table_version"
// The variable name in mysql.tidb table and it records the cluster id of this cluster
tidbClusterID = "cluster_id"
// Const for TiDB server version 2.
version2 = 2
version3 = 3
Expand Down Expand Up @@ -1244,16 +1246,20 @@ const (
// Add max_node_count column to tidb_global_task and tidb_global_task_history.
version224 = 224

// version 225
// insert `cluster_id` into the `mysql.tidb` table.
version225 = 225

// ...
// [version225, version238] is the version range reserved for patches of 8.5.x
// [version226, version238] is the version range reserved for patches of 8.5.x
// ...

// next version should start with 239
)

// 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 = version224
var currentBootstrapVersion int64 = version225

// 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 @@ -1433,6 +1439,7 @@ var (
upgradeToVer222,
upgradeToVer223,
upgradeToVer224,
upgradeToVer225,
}
)

Expand Down Expand Up @@ -3317,6 +3324,30 @@ func upgradeToVer224(s sessiontypes.Session, ver int64) {
doReentrantDDL(s, "ALTER TABLE mysql.tidb_global_task_history ADD COLUMN max_node_count INT DEFAULT 0 AFTER `modify_params`;", infoschema.ErrColumnExists)
}

// writeClusterID writes cluster id into mysql.tidb
func writeClusterID(s sessiontypes.Session) {
ctx, cancel := context.WithTimeout(context.Background(), time.Duration(internalSQLTimeout)*time.Second)
defer cancel()

clusterID := s.GetDomain().(*domain.Domain).GetPDClient().GetClusterID(ctx)

mustExecute(s, `INSERT HIGH_PRIORITY INTO %n.%n VALUES (%?, %?, "TiDB Cluster ID.") ON DUPLICATE KEY UPDATE VARIABLE_VALUE= %?`,
mysql.SystemDB,
mysql.TiDBTable,
tidbClusterID,
clusterID,
clusterID,
)
Comment on lines +3351 to +3364
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Guard the PD client before fetching cluster_id.

(*domain.Domain).GetPDClient() can return nil when the store is not kv.StorageWithPD, so this dereference will panic during bootstrap/upgrade on uni-store/mock-store style deployments.

🛠️ Suggested fix
 // writeClusterID writes cluster id into mysql.tidb
 func writeClusterID(s sessiontypes.Session) {
+	pdClient := domain.GetDomain(s).GetPDClient()
+	if pdClient == nil {
+		logutil.BgLogger().Warn("skip writing cluster_id: PD client unavailable during bootstrap/upgrade")
+		return
+	}
+
 	ctx, cancel := context.WithTimeout(context.Background(), time.Duration(internalSQLTimeout)*time.Second)
 	defer cancel()
 
-	clusterID := s.GetDomain().(*domain.Domain).GetPDClient().GetClusterID(ctx)
+	clusterID := pdClient.GetClusterID(ctx)
 
 	mustExecute(s, `INSERT HIGH_PRIORITY INTO %n.%n VALUES (%?, %?, "TiDB Cluster ID.") ON DUPLICATE KEY UPDATE VARIABLE_VALUE= %?`,
 		mysql.SystemDB,
 		mysql.TiDBTable,

As per coding guidelines, "Keep error handling actionable and contextual; avoid silently swallowing errors in Go code".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/session/bootstrap.go` around lines 3327 - 3340, The writeClusterID
function currently dereferences the PD client returned by
s.GetDomain().(*domain.Domain).GetPDClient() without a nil-check, which can
panic for stores without PD; modify writeClusterID to retrieve the PD client
into a variable, check if pdClient == nil (or the domain assertion fails) and
return early (or log/handle) when nil, and only call pdClient.GetClusterID(ctx)
when non-nil before passing clusterID into mustExecute; reference the symbols
writeClusterID, sessiontypes.Session, s.GetDomain(), domain.Domain,
GetPDClient(), GetClusterID(), mustExecute, and tidbClusterID to locate the
change.

}

func upgradeToVer225(s sessiontypes.Session, ver int64) {
if ver >= version225 {
return
}

writeClusterID(s)
}

// initGlobalVariableIfNotExists initialize a global variable with specific val if it does not exist.
func initGlobalVariableIfNotExists(s sessiontypes.Session, name string, val any) {
ctx := kv.WithInternalSourceType(context.Background(), kv.InternalTxnBootstrap)
Expand Down Expand Up @@ -3568,6 +3599,8 @@ func doDMLWorks(s sessiontypes.Session) {

writeDDLTableVersion(s)

writeClusterID(s)

ctx := kv.WithInternalSourceType(context.Background(), kv.InternalTxnBootstrap)
_, err := s.ExecuteInternal(ctx, "COMMIT")
if err != nil {
Expand Down
55 changes: 55 additions & 0 deletions pkg/session/bootstrap_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2591,3 +2591,58 @@ func TestTiDBUpgradeToVer219(t *testing.T) {
require.Contains(t, string(chk.GetRow(0).GetBytes(1)), "idx_schema_table_state")
require.Contains(t, string(chk.GetRow(0).GetBytes(1)), "idx_schema_table_partition_state")
}

func TestWriteClusterIDToMySQLTiDBWhenUpgradingTo225(t *testing.T) {
ctx := context.Background()
store, dom := CreateStoreAndBootstrap(t)
defer func() { require.NoError(t, store.Close()) }()

// `cluster_id` is inserted for a new TiDB cluster.
se := CreateSessionAndSetID(t, store)
r := MustExecToRecodeSet(t, se, `select VARIABLE_VALUE from mysql.tidb where VARIABLE_NAME='cluster_id'`)
req := r.NewChunk(nil)
err := r.Next(ctx, req)
require.NoError(t, err)
require.Equal(t, 1, req.NumRows())
require.NotEmpty(t, req.GetRow(0).GetBytes(0))
require.NoError(t, r.Close())
se.Close()

// bootstrap as version224
ver224 := version224
seV224 := CreateSessionAndSetID(t, store)
txn, err := store.Begin()
require.NoError(t, err)
m := meta.NewMutator(txn)
err = m.FinishBootstrap(int64(ver224))
require.NoError(t, err)
revertVersionAndVariables(t, seV224, ver224)
// remove the cluster_id entry from mysql.tidb table
MustExec(t, seV224, "delete from mysql.tidb where variable_name='cluster_id'")
err = txn.Commit(ctx)
require.NoError(t, err)
ver, err := getBootstrapVersion(seV224)
require.NoError(t, err)
require.Equal(t, int64(ver224), ver)
seV224.Close()

// upgrade to current version
dom.Close()
domCurVer, err := BootstrapSession(store)
require.NoError(t, err)
defer domCurVer.Close()
seCurVer := CreateSessionAndSetID(t, store)
ver, err = getBootstrapVersion(seCurVer)
require.NoError(t, err)
require.Equal(t, currentBootstrapVersion, ver)

// check if the cluster_id has been set in the `mysql.tidb` table during upgrade
r = MustExecToRecodeSet(t, seCurVer, `select VARIABLE_VALUE from mysql.tidb where VARIABLE_NAME='cluster_id'`)
req = r.NewChunk(nil)
err = r.Next(ctx, req)
require.NoError(t, err)
require.Equal(t, 1, req.NumRows())
require.NotEmpty(t, req.GetRow(0).GetBytes(0))
require.NoError(t, r.Close())
seCurVer.Close()
}