ddl, table: skip pre-check SELECT for widening modify-column changes

When ALTER TABLE ... MODIFY COLUMN is classified as ModifyTypeIndexReorg
(for example char(N) -> varchar(M) with utf8mb4_bin + a secondary index),
the StateDeleteOnly handler runs a synchronous
  SELECT ... FROM t WHERE LENGTH(col) > N OR col IS NULL LIMIT 1
through the internal session pool. On large tables this SELECT has no
timeout, pins the cluster GC safepoint for its entire duration, blocks
subsequent DDL on the same table, and is invisible to SHOW PROCESSLIST.

For a strictly-widening modification (integer range expansion with the
same signedness; char/varchar flen increase) no existing row can possibly
violate the new type. In that case the pre-check is pure overhead and
can be skipped. Nullability tightening (NULL -> NOT NULL) is also safe
to skip because the NotNullFlag guard added to
castIndexValuesToChangingTypes catches existing NULL rows during reorg.

Add dataValuesFitNewType in pkg/ddl/modify_column.go and gate the
pre-check on it. The helper covers the two safe-widening cases; all
other shape changes (length shrink, decimal, temporal, enum/set, ...)
fall through to the conservative default and keep the pre-check.
Collation and signedness mismatches are already routed to
ModifyTypeReorg by getModifyColumnType before this handler runs.

In pkg/table/tables/index.go, extend castIndexValuesToChangingTypes
with a NULL guard right after the strict cast: the underlying
Datum.ConvertTo converts NULL to NULL without error, so when the new
column type carries NotNullFlag the guard is what makes the reorg
reject existing NULL rows and drive rollback.

Add failpoint hook checkModifyColumnDataEntry at the top of
checkModifyColumnData so tests can observe whether the pre-check was
reached.

Tests:

- TestModifyColumnIndexReorgWideningSkipsPrecheck covers the widening
  happy path; checkModifyColumnData is never entered and the ALTER
  succeeds.
- TestModifyColumnIndexReorgWideningCatchesNullInReorg covers the
  widening + NULL -> NOT NULL case; the reorg NULL guard rejects the
  NULL row after the pre-check is skipped.
- TestModifyColumnIndexReorgRangeViolation preserves the narrowing
  failure path (pre-check still runs and catches the oversized row).
- TestModifyColumnIndexReorgNullToNotNull preserves the narrowing +
  nullability-tightening failure path.
- TestModifyColumnIndexReorgRollbackOnReorgPanic pins the reorg-phase
  rollback path end-to-end via errorMockPanic.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: joechenrh

pkg/ddl/modify_column.go

@@ -65,6 +65,23 @@ func isIntegerChange(from, to *model.ColumnInfo) bool {
 	return mysql.IsIntegerType(from.GetType()) && mysql.IsIntegerType(to.GetType())
 }
 
+// dataValuesFitNewType reports whether every stored value of oldCol is
+// guaranteed to fit newCol so the pre-check SQL can be skipped on the
+// ModifyTypeIndexReorg path. Only integer and char/varchar flen widening
+// qualify; NULL -> NOT NULL is still safe because the NotNullFlag guard in
+// CastColumnValueWithStrictMode catches existing NULLs during reorg.
+func dataValuesFitNewType(oldCol, newCol *model.ColumnInfo) bool {
+	if isIntegerChange(oldCol, newCol) {
+		oldDefLen, _ := mysql.GetDefaultFieldLengthAndDecimal(oldCol.GetType())
+		newDefLen, _ := mysql.GetDefaultFieldLengthAndDecimal(newCol.GetType())
+		return newDefLen >= oldDefLen
+	}
+	if isCharChange(oldCol, newCol) {
+		return newCol.GetFlen() >= oldCol.GetFlen()
+	}
+	return false
+}
+
 func isCharChange(from, to *model.ColumnInfo) bool {
 	return types.IsTypeChar(from.GetType()) && types.IsTypeChar(to.GetType())
 }
@@ -811,6 +828,7 @@ func checkModifyColumnData(
 	oldCol, changingCol *model.ColumnInfo,
 	checkValueRange bool,
 ) (checked bool, err error) {
+	failpoint.InjectCall("checkModifyColumnDataEntry", oldCol.Name.L)
 	// Get sessionctx from context resource pool.
 	var sctx sessionctx.Context
 	sctx, err = w.sessPool.Get()
@@ -1199,15 +1217,19 @@ func (w *worker) doModifyColumnIndexReorg(
 		failpoint.InjectCall("modifyColumnTypeWithData", job, args)
 		job.FillArgs(args)
 	case model.StateDeleteOnly:
-		checked, err := checkModifyColumnData(
-			jobCtx.stepCtx, w,
-			dbInfo.Name, tblInfo.Name,
-			oldCol, args.Column, true)
-		if err != nil {
-			if checked {
-				job.State = model.JobStateRollingback
+		// Skip the pre-check SELECT when the new type strictly widens the
+		// old one; reorg-time cast covers NULL -> NOT NULL.
+		if !dataValuesFitNewType(oldCol, args.Column) {
+			checked, err := checkModifyColumnData(
+				jobCtx.stepCtx, w,
+				dbInfo.Name, tblInfo.Name,
+				oldCol, args.Column, true)
+			if err != nil {
+				if checked {
+					job.State = model.JobStateRollingback
+				}
+				return ver, errors.Trace(err)
 			}
-			return ver, errors.Trace(err)
 		}
 
 		// delete only -> write only

pkg/ddl/modify_column_test.go

@@ -683,6 +683,269 @@ func TestModifyColumnWithSkipReorg(t *testing.T) {
 	require.Equal(t, model.ModifyTypeNoReorgWithCheck, gotTp)
 }
 
+// TestModifyColumnIndexReorgRollbackOnReorgPanic verifies a reorg-phase
+// failure (errorMockPanic -> ErrReorgPanic) on the ModifyTypeIndexReorg path
+// rolls back cleanly.
+func TestModifyColumnIndexReorgRollbackOnReorgPanic(t *testing.T) {
+	store := testkit.CreateMockStore(t)
+	tk := testkit.NewTestKit(t, store)
+	tk.MustExec("use test")
+
+	// Cap the retry count so the panic loop terminates quickly.
+	tk.MustExec("set @@global.tidb_ddl_error_count_limit = 3")
+	defer tk.MustExec("set @@global.tidb_ddl_error_count_limit = default")
+
+	// char(N) collate utf8mb4_bin with index -> varchar(M) collate utf8mb4_bin
+	// routes to ModifyTypeIndexReorg because types.NeedRestoredData differs
+	// between CHAR+bin (false) and VARCHAR+bin (true). See pkg/types/etc.go:143.
+	tk.MustExec(`create table t (
+		a char(20) collate utf8mb4_bin,
+		key i1(a)
+	) default charset=utf8mb4 default collate=utf8mb4_bin`)
+	// All values fit varchar(10) (LENGTH <= 10) so the StateDeleteOnly
+	// pre-check passes and the job reaches the reorg phase, where the
+	// injected panic will fire.
+	tk.MustExec("insert into t values ('short'), ('medlen_abc'), ('abc')")
+
+	// Capture which modify-column type is selected so the test asserts we are
+	// actually exercising ModifyTypeIndexReorg rather than a sibling path.
+	var gotTp byte
+	testfailpoint.EnableCall(t, "github.com/pingcap/tidb/pkg/ddl/getModifyColumnType", func(tp byte) {
+		gotTp = tp
+	})
+
+	// Force the reorg phase to panic on every batch. util.Recover in
+	// pkg/ddl/backfilling.go converts the panic to dbterror.ErrReorgPanic,
+	// which terminates the reorg and triggers rollback.
+	require.NoError(t, failpoint.Enable("github.com/pingcap/tidb/pkg/ddl/errorMockPanic", `return(true)`))
+	defer func() {
+		require.NoError(t, failpoint.Disable("github.com/pingcap/tidb/pkg/ddl/errorMockPanic"))
+	}()
+
+	err := tk.ExecToErr("alter table t modify column a varchar(10) collate utf8mb4_bin")
+	require.Error(t, err, "expected ALTER to fail due to injected reorg panic")
+	t.Logf("observed error: %v", err)
+
+	require.Equalf(t, model.ModifyTypeIndexReorg, gotTp,
+		"expected the modify-column job to be classified as ModifyTypeIndexReorg; got %d", gotTp)
+
+	// Rollback must leave the column unchanged.
+	tblInfo := external.GetTableByName(t, tk, "test", "t").Meta()
+	col := tblInfo.FindPublicColumnByName("a")
+	require.NotNil(t, col, "column a should still exist after rollback")
+	require.Equalf(t, mysql.TypeString, col.GetType(),
+		"expected column a to still be CHAR (TypeString); got type=%d", col.GetType())
+	require.Equal(t, 20, col.GetFlen(), "expected column a to still have flen=20")
+	require.Nil(t, col.ChangingFieldType, "ChangingFieldType should be cleared after rollback")
+	require.False(t, mysql.HasPreventNullInsertFlag(col.GetFlag()),
+		"PreventNullInsertFlag should be cleared after rollback")
+
+	// No lingering "changing" temp indexes.
+	for _, idx := range tblInfo.Indices {
+		for _, idxCol := range idx.Columns {
+			require.Falsef(t, idxCol.UseChangingType,
+				"lingering changing index column detected: index=%s col=%s", idx.Name.L, idxCol.Name.L)
+		}
+	}
+
+	// Physical consistency check on KV + indexes.
+	tk.MustExec("admin check table t")
+
+	// Original data must remain readable and complete.
+	tk.MustQuery("select a from t order by a").
+		Check(testkit.Rows("abc", "medlen_abc", "short"))
+}
+
+// TestModifyColumnIndexReorgRangeViolation: narrowing with an oversized row
+// falls through to the pre-check (dataValuesFitNewType=false), which catches
+// the violation and rolls back.
+func TestModifyColumnIndexReorgRangeViolation(t *testing.T) {
+	store := testkit.CreateMockStore(t)
+	tk := testkit.NewTestKit(t, store)
+	tk.MustExec("use test")
+
+	tk.MustExec(`create table t (
+		a char(20) collate utf8mb4_bin,
+		key i1(a)
+	) default charset=utf8mb4 default collate=utf8mb4_bin`)
+	// 'too_long_to_fit_varchar_10' is 26 bytes, over the target varchar(10)
+	// limit. The cast in castIndexValuesToChangingTypes must reject it.
+	tk.MustExec("insert into t values ('ok'), ('too_long_to_fit_varchar_10')")
+
+	var gotTp byte
+	testfailpoint.EnableCall(t, "github.com/pingcap/tidb/pkg/ddl/getModifyColumnType", func(tp byte) {
+		gotTp = tp
+	})
+
+	err := tk.ExecToErr("alter table t modify column a varchar(10) collate utf8mb4_bin")
+	require.Error(t, err, "expected ALTER to fail due to over-length row")
+	t.Logf("observed error: %v", err)
+
+	require.Equalf(t, model.ModifyTypeIndexReorg, gotTp,
+		"expected ModifyTypeIndexReorg; got %d", gotTp)
+
+	tblInfo := external.GetTableByName(t, tk, "test", "t").Meta()
+	col := tblInfo.FindPublicColumnByName("a")
+	require.NotNil(t, col)
+	require.Equal(t, mysql.TypeString, col.GetType(), "column should still be CHAR after rollback")
+	require.Equal(t, 20, col.GetFlen(), "column flen should be restored to 20")
+	require.Nil(t, col.ChangingFieldType, "ChangingFieldType should be cleared")
+	for _, idx := range tblInfo.Indices {
+		for _, idxCol := range idx.Columns {
+			require.Falsef(t, idxCol.UseChangingType,
+				"lingering changing index column: index=%s col=%s", idx.Name.L, idxCol.Name.L)
+		}
+	}
+	tk.MustExec("admin check table t")
+	tk.MustQuery("select a from t order by a").
+		Check(testkit.Rows("ok", "too_long_to_fit_varchar_10"))
+}
+
+// TestModifyColumnIndexReorgNullToNotNull: narrowing + NULL -> NOT NULL with
+// an existing NULL row. Pre-check runs (flen shrink) and catches it; the
+// widening counterpart is TestModifyColumnIndexReorgWideningCatchesNullInReorg.
+func TestModifyColumnIndexReorgNullToNotNull(t *testing.T) {
+	store := testkit.CreateMockStore(t)
+	tk := testkit.NewTestKit(t, store)
+	tk.MustExec("use test")
+
+	// char(20) nullable + index -> varchar(10) NOT NULL triggers
+	// ModifyTypeIndexReorg (CHAR/VARCHAR NeedRestoredData mismatch) and
+	// also crosses the NULL -> NOT NULL boundary.
+	tk.MustExec(`create table t (
+		a char(20) collate utf8mb4_bin null,
+		key i1(a)
+	) default charset=utf8mb4 default collate=utf8mb4_bin`)
+	tk.MustExec("insert into t values ('ok'), (NULL)")
+
+	var gotTp byte
+	testfailpoint.EnableCall(t, "github.com/pingcap/tidb/pkg/ddl/getModifyColumnType", func(tp byte) {
+		gotTp = tp
+	})
+
+	err := tk.ExecToErr("alter table t modify column a varchar(10) collate utf8mb4_bin not null")
+	require.Error(t, err, "expected ALTER to fail due to NULL row on NOT NULL target")
+	t.Logf("observed error: %v", err)
+
+	require.Equalf(t, model.ModifyTypeIndexReorg, gotTp,
+		"expected ModifyTypeIndexReorg; got %d", gotTp)
+
+	tblInfo := external.GetTableByName(t, tk, "test", "t").Meta()
+	col := tblInfo.FindPublicColumnByName("a")
+	require.NotNil(t, col)
+	require.Equal(t, mysql.TypeString, col.GetType(), "column should still be CHAR after rollback")
+	require.Equal(t, 20, col.GetFlen(), "column flen should be restored")
+	require.False(t, mysql.HasNotNullFlag(col.GetFlag()), "NotNullFlag should be cleared on rollback")
+	require.Nil(t, col.ChangingFieldType, "ChangingFieldType should be cleared")
+	require.False(t, mysql.HasPreventNullInsertFlag(col.GetFlag()),
+		"PreventNullInsertFlag should be cleared")
+	for _, idx := range tblInfo.Indices {
+		for _, idxCol := range idx.Columns {
+			require.Falsef(t, idxCol.UseChangingType,
+				"lingering changing index column: index=%s col=%s", idx.Name.L, idxCol.Name.L)
+		}
+	}
+	tk.MustExec("admin check table t")
+	// The NULL row must still be present (ALTER rolled back, data intact).
+	tk.MustQuery("select count(*) from t where a is null").Check(testkit.Rows("1"))
+}
+
+// TestModifyColumnIndexReorgWideningCatchesNullInReorg: widening + NULL ->
+// NOT NULL. Pre-check is skipped (dataValuesFitNewType=true); the NotNullFlag
+// guard in CastColumnValueWithStrictMode catches the NULL row during reorg.
+func TestModifyColumnIndexReorgWideningCatchesNullInReorg(t *testing.T) {
+	store := testkit.CreateMockStore(t)
+	tk := testkit.NewTestKit(t, store)
+	tk.MustExec("use test")
+
+	tk.MustExec(`create table t (
+		a char(60) collate utf8mb4_bin null,
+		key i1(a)
+	) default charset=utf8mb4 default collate=utf8mb4_bin`)
+	tk.MustExec("insert into t values ('ok'), (NULL)")
+
+	var gotTp byte
+	testfailpoint.EnableCall(t, "github.com/pingcap/tidb/pkg/ddl/getModifyColumnType", func(tp byte) {
+		gotTp = tp
+	})
+	precheckInvoked := false
+	testfailpoint.EnableCall(t, "github.com/pingcap/tidb/pkg/ddl/checkModifyColumnDataEntry", func(string) {
+		precheckInvoked = true
+	})
+
+	err := tk.ExecToErr("alter table t modify column a varchar(70) collate utf8mb4_bin not null")
+	require.Error(t, err, "expected ALTER to fail due to NULL row on NOT NULL target")
+	t.Logf("observed error: %v", err)
+
+	require.Equalf(t, model.ModifyTypeIndexReorg, gotTp,
+		"expected ModifyTypeIndexReorg; got %d", gotTp)
+	require.False(t, precheckInvoked,
+		"expected the pre-check SELECT to be skipped (widening case); reorg cast should catch the NULL")
+
+	tblInfo := external.GetTableByName(t, tk, "test", "t").Meta()
+	col := tblInfo.FindPublicColumnByName("a")
+	require.NotNil(t, col)
+	require.Equal(t, mysql.TypeString, col.GetType(), "column should still be CHAR after rollback")
+	require.Equal(t, 60, col.GetFlen(), "column flen should be restored")
+	require.False(t, mysql.HasNotNullFlag(col.GetFlag()), "NotNullFlag should be cleared on rollback")
+	require.Nil(t, col.ChangingFieldType, "ChangingFieldType should be cleared")
+	require.False(t, mysql.HasPreventNullInsertFlag(col.GetFlag()),
+		"PreventNullInsertFlag should be cleared")
+	for _, idx := range tblInfo.Indices {
+		for _, idxCol := range idx.Columns {
+			require.Falsef(t, idxCol.UseChangingType,
+				"lingering changing index column: index=%s col=%s", idx.Name.L, idxCol.Name.L)
+		}
+	}
+	tk.MustExec("admin check table t")
+	tk.MustQuery("select count(*) from t where a is null").Check(testkit.Rows("1"))
+}
+
+// TestModifyColumnIndexReorgWideningSkipsPrecheck: strictly-widening change
+// (char(60) -> varchar(70)) skips the pre-check and the ALTER succeeds.
+func TestModifyColumnIndexReorgWideningSkipsPrecheck(t *testing.T) {
+	store := testkit.CreateMockStore(t)
+	tk := testkit.NewTestKit(t, store)
+	tk.MustExec("use test")
+
+	tk.MustExec(`create table t (
+		a char(60) collate utf8mb4_bin,
+		key i1(a)
+	) default charset=utf8mb4 default collate=utf8mb4_bin`)
+	tk.MustExec("insert into t values ('short'), ('medium length value'), ('something else')")
+
+	var gotTp byte
+	testfailpoint.EnableCall(t, "github.com/pingcap/tidb/pkg/ddl/getModifyColumnType", func(tp byte) {
+		gotTp = tp
+	})
+
+	// checkModifyColumnDataEntry fires at the top of checkModifyColumnData.
+	// For a widening modify column we expect it to never fire.
+	precheckInvoked := false
+	testfailpoint.EnableCall(t, "github.com/pingcap/tidb/pkg/ddl/checkModifyColumnDataEntry", func(colName string) {
+		precheckInvoked = true
+		t.Logf("pre-check entered for column %q", colName)
+	})
+
+	tk.MustExec("alter table t modify column a varchar(70) collate utf8mb4_bin")
+
+	require.Equalf(t, model.ModifyTypeIndexReorg, gotTp,
+		"expected ModifyTypeIndexReorg; got %d", gotTp)
+	require.False(t, precheckInvoked,
+		"expected the pre-check SELECT to be skipped for widening char -> varchar")
+
+	meta := external.GetTableByName(t, tk, "test", "t").Meta()
+	col := meta.FindPublicColumnByName("a")
+	require.NotNil(t, col)
+	require.Equal(t, mysql.TypeVarchar, col.GetType(), "column should now be VARCHAR")
+	require.Equal(t, 70, col.GetFlen(), "column flen should be 70")
+	require.Nil(t, col.ChangingFieldType, "ChangingFieldType should be cleared after completion")
+
+	tk.MustExec("admin check table t")
+	tk.MustQuery("select a from t order by a").
+		Check(testkit.Rows("medium length value", "short", "something else"))
+}
+
 func TestGetModifyColumnType(t *testing.T) {
 	type testCase struct {
 		beforeType string

pkg/table/tables/index.go

@@ -137,6 +137,11 @@ func (c *index) castIndexValuesToChangingTypes(indexedValues []types.Datum) erro
 		if err != nil {
 			return err
 		}
+		// Strict cast passes NULL through; reject it here when the new type
+		// carries NotNullFlag so reorg surfaces existing NULL rows.
+		if indexedValues[i].IsNull() && mysql.HasNotNullFlag(tblCol.ChangingFieldType.GetFlag()) {
+			return table.ErrColumnCantNull.FastGenByArgs(tblCol.Name)
+		}
 	}
 	return nil
 }