pingcap / pingcap/tidb

ddl: MODIFY COLUMN rolls back when column name contains backticks (unescaped identifier in internal check SQL)

Open
#70,285 0 comments 0 reactions 0 assignees View on GitHub
affects-8.5 component/ddl severity/major type/bug
Dominant language
Go
Stars
40.5k
Forks
6.2k
PR merge metrics
PR metrics pending

Description

## Bug Report

**Affected versions**: master (since #63965) and release-8.5 v8.5.4+ (cherry-pick #64138). Found during a PR-by-PR review of release-8.5.

### 1. Minimal reproduce step (Required)

```sql
DROP DATABASE IF EXISTS d;
CREATE DATABASE d;

CREATE TABLE d.t (`a``b` INT NOT NULL); -- column name is a`b
INSERT INTO d.t VALUES (1);

ALTER TABLE d.t MODIFY COLUMN `a``b` TINYINT NOT NULL;
SHOW CREATE TABLE d.t; -- column type is unchanged (still INT)
```

### 2. What did you expect to see? (Required)

The existing value `1` fits in `TINYINT`, so the no-reorg MODIFY COLUMN path should validate the data and succeed:

```
`a``b` tinyint NOT NULL
```

### 3. What did you see instead (Required)

The DDL job rolls back with a syntax error from the internally generated check SQL:

```
DDL job rollback
You have an error in your SQL syntax ... near "` FROM `d`.`t` WHERE (`a`b` < -128 OR `a`b` > 127) LIMIT 1"
```

and the column type is unchanged (`int`). The generated internal SQL contains `` `a`b` `` — the backtick inside the identifier is not escaped, producing unparseable SQL.

### 4. What is your TiDB version? (Required)

Reproduced on v8.5.7 (release-8.5, git 1fdc13626a). Root cause confirmed by code inspection in master @ 6f5bfe198f (2026-08-01), which carries identical code.

### Root cause analysis

The no-row-reorg MODIFY COLUMN optimization (#63965, later widened by #63465) generates an internal SQL statement to check whether existing data fits the new column type. `buildCheckSQLFromModifyColumn` (`pkg/ddl/modify_column.go:851-852` on release-8.5, `:862-864` on master) wraps identifiers with plain string concatenation:

```go
checkColName := fmt.Sprintf("`%s`", oldCol.Name.O)
tableName := fmt.Sprintf("`%s`.`%s`", dbName.O, tblName.O)
```

Backticks inside the identifier are never escaped, so a column named `` a`b `` produces `` SELECT `a`b` FROM ... `` — invalid SQL. `checkModifyColumnData` (`modify_column.go:816`) executes it via `ExecRestrictedSQL` and any error aborts the job, causing the rollback. Note the schema/table names are concatenated the same way, so a database or table name containing a backtick is affected identically.

Introduced by b1f10c8ec5 on release-8.5 (master a9bb0133e2, PR #63965; cherry-pick #64138); 40946f950b (#63465, cherry-pick #64123) later extended this fast path to more cases while keeping the unescaped construction.

Suggested direction: escape embedded backticks when quoting (e.g. `strings.ReplaceAll(name, "`", "``")`), or build the check statement through the AST/SQL builder instead of raw string concatenation.

Related: #63965 (original PR), #64123 (path widening), #63595 (tracking issue for skip-reorg MODIFY COLUMN).

Contributor guide

Open the contributing guide

Research direction

Start in pkg/ddl/modify_column.go, especially buildCheckSQLFromModifyColumn and checkModifyColumnData, and reproduce the ALTER TABLE case with a backtick-containing column name. Trace the generated check SQL and add regression coverage for escaped identifiers, including database and table names if appropriate. Done means the MODIFY COLUMN succeeds and SHOW CREATE TABLE reports the new type without rollback.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, mysql, sql
Domain
databases
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
75/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.