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.

getModifyColumnType only routes isCharChange cases into
ModifyTypeIndexReorg (integer signedness flips go to ModifyTypeReorg,
same-signedness integer changes resolve to noReorgDataStrict or
NoReorgWithCheck; decimal, temporal, enum/set are similarly handled
before this path). So the only widening case that reaches this handler
is char/varchar flen widening, and for that case no existing row can
possibly violate the new type: the pre-check is pure overhead.

Gate the pre-check in doModifyColumnIndexReorg's StateDeleteOnly branch
on `isCharChange(oldCol, newCol) && newCol.GetFlen() >= oldCol.GetFlen()`;
otherwise keep the existing pre-check.

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 this guard is what protects the race
window between the statement-build NULL check (GetModifiableColumnJob
calls checkForNullValue) and the DDL job's StateNone handler setting
PreventNullInsertFlag. A NULL row that lands in that window would
otherwise slip through the reorg silently.

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.
- TestModifyColumnIndexReorgRangeViolation preserves the narrowing
  failure path (pre-check still runs and catches the oversized row).
- 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

@@ -811,6 +811,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 +1200,23 @@ 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 for char/varchar flen widening: the
+		// ModifyTypeIndexReorg path is only reachable via isCharChange in
+		// getModifyColumnType, so this is the one case where no existing
+		// row can violate. NULL -> NOT NULL is caught by the NotNullFlag
+		// guard in castIndexValuesToChangingTypes during reorg.
+		widensFlen := isCharChange(oldCol, args.Column) && args.Column.GetFlen() >= oldCol.GetFlen()
+		if !widensFlen {
+			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,169 @@ 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
+// still runs the pre-check (not a widening), 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`)
+	// 'abcdefghijklmno' is 15 bytes: fits char(20) on insert, but violates
+	// the target varchar(10) limit during the pre-check / reorg cast.
+	tk.MustExec("insert into t values ('ok'), ('abcdefghijklmno')")
+
+	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("abcdefghijklmno", "ok"))
+}
+
+// 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
 }