pingcap / pingcap/tidb

KILL QUERY on a queued duplicate `ADD INDEX` rolls back a `public` index created by another job

Open
#71,046 2 comments 0 reactions 0 assignees View on GitHub
affects-25.10 affects-26.3 affects-7.5 affects-8.1 affects-8.5 affects-9.0 component/ddl severity/critical type/bug
Dominant language
Go
Stars
40.5k
Forks
6.2k
PR merge metrics
PR metrics pending

Description

# KILL QUERY on a queued duplicate `ADD INDEX` rolls back a `public` index created by another job

## Bug Report

A DDL job does not own the index it rolls back. When a duplicate `ADD INDEX` job is cancelled while it is still `queueing`, `convertNotReorgAddIdxJob2RollbackJob` looks the index up **by name** in the current `TableInfo` and does not exclude an index that another job already published. It therefore drives the *other* job's `public` index into `delete only` and eventually drops it, including its delete-range.

This is a wrong-result class bug: after the index is gone, the table data is still there (the row stays visible in a table scan), any subsequent indexed-column update deletes the old index entry without writing a new one, and queries that use that index start missing rows. Pinning it to a commit is only possible with bit-level control of the DDL job lifecycle, which is what the new regression test does.

### 1. Minimal reproduce step (Required)

At a glance, on the failing build `f2c346fe4f368ff855e17c1f62e28a89ba7f9723`:

```text
repro : two identical ADD INDEX jobs are queued; the later one gets KILL QUERY
before it runs, the earlier one publishes the index first
expected : the queued duplicate job is cancelled; the published index stays public
actual : the queued duplicate job rolls back the *other* job's public index
(delete only -> delete-range -> index gone), the row still exists
verified : 70b0c5d4 10/10 FAIL, 6eff5759 (its parent) 10/10 PASS,
65ac2fad and f2c346fe 10/10 FAIL; no_cancel PASS in all four builds
```

The scenario needs a DDL job that is cancelled while `queueing`, and another job that has already published the same-named index. The test [`TestDuplicateAddIndexQueuedCancellation/kill_query`](https://github.com/zyguan/tidb/blob/d811515d7e1bf0f8fb27a13eec9a9326b5290035/pkg/ddl/tests/metadatalock/duplicate_index_cancel_test.go) constructs exactly that on a mock store + real mock server, without sleeps:

1. Park the owner scheduler at the existing `beforeLoadAndDeliverJobs` failpoint, so jobs are persisted to `mysql.tidb_ddl_job` but cannot be dispatched.
2. Session J1 submits `create index idx_dup_cancel on test.duplicate_index_cancel(k, v)`.
3. Session J2 submits the **same** statement. Both pass the front-end duplicate-index check, because neither job has run yet and no index metadata exists. Two distinct job IDs on the same table, same index name, both `queueing/none`.
4. Deliver the kill to J2's connection while J2 is still queued, and confirm the signal really arrived (`SQLKiller.GetKillSignal() == QueryInterrupted`, connection not shut down). See the note below on why the harness uses the server-side kill call.
5. Release the scheduler and let J1 finish first: it publishes index ID 1 as `public`.
6. Only then let J2 take its first job step (it is parked at the existing `beforeRunOneJobStep` failpoint).

The repro code is linked at the end of this report; raw per-commit logs and the cross-experiment results are summarized in sections 3 and 4.

> Harness note: the test delivers the kill through `Server.Kill(connID, query=true, ...)`, which is the server-side path a SQL `KILL QUERY` reaches after parsing, privilege checks and global connection-ID routing. The SQL text form cannot be used in this mock/NextGen harness (it raises a nil pointer dereference and leaves the target signal at 0), so this experiment proves the DDL cancellation chain but is not a SQL-layer end-to-end test.

The repro in SQL terms (what the test does, step by step)

```sql
-- setup (once)
create table test.duplicate_index_cancel (id int primary key, k int, v int);
insert into test.duplicate_index_cancel values (1, 1, 1);

-- J1 and J2 both run this, while the DDL scheduler is parked and no index exists yet:
create index idx_dup_cancel on test.duplicate_index_cancel(k, v);
-- => two queued jobs, different job IDs, same table, same index name

-- J2's connection is killed while its job is still queueing:
kill query ;

-- J1 is released first and publishes the index; then J2 takes its first step.
```

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

`KILL QUERY` on the queued duplicate `ADD INDEX` job cancels **that** job only.

The index published by J1 must keep its identity and state: the entry for index ID 1 must still exist in the table metadata with `StatePublic`, and the row must remain readable through that index:

```sql
select v from test.duplicate_index_cancel use index(idx_dup_cancel) where k = 1;
-- expect: 1
```

The duplicate job may legitimately fail with `[ddl:1061] index already exist idx_dup_cancel`, but it must not modify metadata it did not create.

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

The index is rolled back and disappears. On `f2c346fe`, J2 goes to `cancelling`, then `rollingback / StateDeleteOnly`, then `rollback done`, and registers a delete-range that covers **J1's** index:

```text
run one job step jobID=11 ... State:cancelling, SchemaState:none
convert/rollback jobID=11 ... State:rollingback, SchemaState:delete only
afterRunOneJobStep_11_state="rollback done"
victimHistoryState="rollback done"
delRange emulator complete task jobID=11 elementID=2 startKey=7480000000000000085f69ffff000000000001 endKey=...0002
```

The final table metadata no longer contains index ID 1, while the table row is still present. The test fails at the index-identity assertion:

```text
Messages: the index published by the first job no longer exists;
events: publishedIndexID=1 publishedIndexState="public" victimJobID=11
victimHistoryState="rollback done" victimSQLError="[ddl:8214]Cancelled DDL job"
```

On the test matrix, the `no_cancel` control passes in all versions (the duplicate job fails with `1061` and the original index survives), and the `admin_cancel` control fails in all versions, which shows the rollback defect is independent of the `KILL` entry point.

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

Failure build: `f2c346fe4f368ff855e17c1f62e28a89ba7f9723` (2026-09-04, Next Generation Kernel, `Kernel Type=Next Generation`, MDL always enabled).

The regression was narrowed with a single-variable cross experiment:

| Commit | Role | `kill_query` (`-count=10`) | `no_cancel` | `admin_cancel` |
| --- | --- | --- | --- | --- |
| `70b0c5d4` (`ddl, executor: fix DDL and BRIE task cancellation on KILL (#70508)`) | KILL fix | 10/10 FAIL | PASS | FAIL |
| `6eff5759` (`70b0c5d4^`) | parent | 10/10 PASS | PASS | FAIL |
| `65ac2fad` | previous weekly build | 10/10 FAIL | PASS | FAIL |
| `f2c346fe` | failing build | 10/10 FAIL | PASS | FAIL |
| `6eff5759` + only the `==` → `.Equal()` line changed | cross experiment | 10/10 FAIL | - | - |
| `70b0c5d4` + only that line reverted to `==` | cross experiment | 10/10 PASS | - | - |

### 5. Analysis

The rollback path for a not-yet-started `ADD INDEX` job resolves its target by name only:

- `pkg/ddl/job_worker.go:runOneJobStep` checks `job.IsCancelling()` first and calls `convertJob2RollbackJob`, which dispatches `model.ActionAddIndex` to `rollingbackAddIndex` (`pkg/ddl/rollingback.go`).
- `pkg/ddl/rollingback.go:convertNotReorgAddIdxJob2RollbackJob` collects `tblInfo.FindIndexByName(a.IndexName.L)` and hands the result to `convertAddIdxJob2RollbackJob`, which unconditionally sets `indexInfo.State = model.StateDeleteOnly` for every name hit.
- A name hit is not a "this index belongs to this job" proof. The normal creation path `pkg/ddl/index.go:checkAndBuildIndexInfo` rejects a same-named `public` index, but the rollback path never got the equivalent check.
- This defect predates the KILL fix: the `admin_cancel` control fails on the parent commit as well.

The KILL fix turned a previously dead branch into a reachable one:

- `6eff5759` used sentinel pointer comparison, `sessVars.SQLKiller.HandleSignal() == exeerrors.ErrQueryInterrupted`, while `pkg/util/sqlkiller/sqlkiller.go:getKillError` returns a fresh `GenWithStackByArgs()` value for `QueryInterrupted`. The pointers never match, so the cancellation branch in `DoDDLJobWrapper` never ran, and a killed queued job simply continued as a normal duplicate job (index preserved).
- `70b0c5d4` changed that to semantic equality, `exeerrors.ErrQueryInterrupted.Equal(...)`, which does match, and also made the cancellation actually happen. The queued duplicate job now reaches the rollback conversion above and deletes the published index.
- Conclusion: `70b0c5d4` introduced a correctness regression in the `KILL QUERY` path by activating a pre-existing rollback defect. It did not introduce the rollback defect itself.

The cross experiment above shows that the behavior flip can be explained by that single comparison line, so it does not depend on the other cancellation-retry reordering in the same commit.

### 6. Impact and scope

- Trigger: two concurrent identical `ADD INDEX` statements (for example a client setup retry or two application instances racing `CREATE TABLE IF NOT EXISTS` + `ADD INDEX`), the later job queued and then cancelled by `KILL QUERY`, with the earlier job completing the index first.
- Background: this shape was hit by an internal Jepsen run that executes the test suite many thousands of times per week. In that run, setup created the same table and index concurrently from 15 workers: job 48 published `cycle_sk` as `public`, duplicate job 57 was queued and its connection received `KILL QUERY 1103101960`, and job 57 then entered `delete only` and registered a delete-range for the same table/index IDs. The workload's read transaction then used the old `public` index and missed rows that a table scan still returned. The metadata-destruction step of that sequence is what this report reproduces deterministically; whether the read-path symptom needs additional multi-node conditions is tracked separately below.
- Effect: a `public` secondary index silently becomes unusable and eventually absent, so index reads miss rows that a table scan still returns.
- Not yet closed: the end-to-end read-path consequence in a real multi-node cluster (stale `InfoSchema` node serving the old `public` index while another node's `delete-only` update removes index keys, plus MVCC evidence) has not been reproduced locally.

### 7. Attachments

Repro code is pushed as a test-only branch (not a fix): [zyguan/tidb @ fix/duplicate-add-index-cancel-rollback](https://github.com/zyguan/tidb/tree/fix/duplicate-add-index-cancel-rollback), commit [`d811515d7e`](https://github.com/zyguan/tidb/commit/d811515d7e1bf0f8fb27a13eec9a9326b5290035), base `origin/master` (`bd762047ca`).

- Regression test: [`pkg/ddl/tests/metadatalock/duplicate_index_cancel_test.go`](https://github.com/zyguan/tidb/blob/d811515d7e1bf0f8fb27a13eec9a9326b5290035/pkg/ddl/tests/metadatalock/duplicate_index_cancel_test.go), `TestDuplicateAddIndexQueuedCancellation` with subtests `kill_query`, `no_cancel`, `admin_cancel`. The test asserts the index identity/state contract and fails on the broken builds.
- Instrumentation: two test-only failpoints around the original kill check in `pkg/ddl/executor.go` (`beforeJepsenQueuedDDLKillCheck`, `afterJepsenQueuedDDLKillCheck`). The comparison expression and the rollback branch are unchanged.
- Environment: `GOTOOLCHAIN=go1.26.0`, tags `intest,deadlock,nextgen`, single Domain + `testkit.CreateMockStoreAndDomain` + `server.CreateMockServer`. The test was verified on `origin/master` (`bd762047ca`): `kill_query` FAIL, `no_cancel` PASS, `admin_cancel` FAIL.
- Limitations: the harness routes the kill through `Server.Kill(connID, query=true, ...)` (the server-side path a SQL `KILL QUERY` reaches after parsing and privilege checks), because the SQL form is unusable in the mock/NextGen harness.

Contributor guide

Open the contributing guide

Research direction

Run pkg/ddl/tests/metadatalock/duplicate_index_cancel_test.go, especially TestDuplicateAddIndexQueuedCancellation and its kill_query subtest, to reproduce the failure. Read pkg/ddl/job_worker.go, pkg/ddl/rollingback.go, pkg/ddl/index.go, and pkg/ddl/executor.go around the entry points and rollback conversion described in the report. Done means the queued duplicate cancellation no longer changes the already-public index, while the no_cancel and admin_cancel controls retain their expected behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, sql
Domain
databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.