pingcap / pingcap/tidb

Fast ADD UNIQUE INDEX can miss build-window duplicates after GC trims MVCC history

Open
#70,591 0 comments 0 reactions 1 assignee Claimed by @wjhuang2016 View on GitHub
component/ddl found-by-ai may-affects-25.10 may-affects-26.3 may-affects-7.5 may-affects-8.1 may-affects-8.5 severity/critical type/bug
Dominant language
Go
Stars
40.5k
Forks
6.2k
PR merge metrics
PR metrics pending

Description

# TiDB fast ADD UNIQUE INDEX can miss concurrent duplicates after GC trims the build-window MVCC history

## Version

- current master around `2026-08-20`

## Summary

Fast online `ADD UNIQUE INDEX` in the ingest path relies on a remote duplicate check that only scans index-key versions with `commit_ts > job.RealStartTS`.

That check runs after ingest completion. If the job runs longer than the default `tidb_gc_life_time = 10m0s`, and a concurrent DML-created conflicting index version from early in the build window is no longer visible to TiKV duplicate detection, the duplicate check can return success even though it should have reported a duplicate-key conflict.

This is dangerous because concurrent user DML during the full-copy preparation period is a normal supported path for the fast add-index design.

On classic TiDB, `tidb_ddl_enable_fast_reorg` is default-enabled, so this is not an exotic opt-in path.

## Expected behavior

`ALTER TABLE ... ADD UNIQUE INDEX ...` should fail with duplicate-key error whenever a concurrent committed DML produces a surviving duplicate unique value during the online build window.

GC/history trimming must not be able to turn a real duplicate into a false-success result.

## Actual behavior

The duplicate-check path is a history-window consumer with no obvious GC/safepoint protection owner:

- TiDB DDL ingest calls `CollectRemoteDuplicateRows` with `MinCommitTS = job.RealStartTS`.
- TiKV duplicate detection starts from versions with `commit_ts > min_commit_ts`.
- When older versions of the same key are no longer considered valid at the boundary, the detector can stop without returning duplicate pairs.

This creates a false-negative shape:

1. online add unique index starts
2. concurrent user DML writes a conflicting unique-index entry during the build window
3. the build lasts beyond default GC retention
4. remote duplicate check no longer sees all versions it needs
5. DDL may succeed instead of returning duplicate-key error

## Why this is production-reachable

- concurrent user DML during fast add-index backfill is a normal supported online-DDL path
- default `tidb_gc_life_time` is only `10m0s`
- duplicate check runs after ingest completion, so long-running builds naturally enlarge the vulnerable window
- fast reorg / ingest add-index is default-enabled on classic TiDB
- other long/history-sensitive paths already protect themselves:
- Lightning duplicate handling uses a service safe-point keeper
- checksum raises GC lifetime to `100h`
- this DDL duplicate-check path appears to do neither

## Evidence

### TiDB side

- `pkg/ddl/ingest/backend.go`: remote duplicate check passes `MinCommitTS = bc.initTS`
- `pkg/ddl/ingest/backend_mgr.go`: `initTS = job.RealStartTS`
- `pkg/ddl/backfilling.go` + `pkg/ddl/ingest/engine_mgr.go`: duplicate check runs in `FinishAndUnregisterEngines(... | OptCheckDup)` after ingest

### TiKV side

- `src/import/duplicate_detect.rs`
- `move_to_next_import_key` starts only at `commit_ts > min_commit_ts`
- `collect_current_key_duplicate` can stop and skip remaining older versions when the boundary/gc-fence condition says they are no longer valid
- `test_duplicate_detect_incremental` documents incremental duplicate-detect semantics across the importer boundary

### Existing intended behavior

- existing TiDB tests in `tests/realtikvtest/addindextest3/temp_index_test.go` expect a duplicate-key error when a concurrent insert at `beforeBackendIngest` creates a duplicate during unique-index build
- that same test shape makes the consequence oracle clear: if history trimming causes the duplicate detector to miss the older build-window version, the DDL can flip from intended duplicate failure into false success

### Temporary mechanism proof

A temporary unit proof on TiDB-side mocked `DuplicateDetect` showed:

- if the stream returns two versions of the same unique-index key with different values, `CollectRemoteDuplicateRows` reports duplicate
- if the stream returns only the latest version, the same path returns `hasDupe=false, err=nil`

This proof was used only to validate mechanism and was not left in the product tree.

## Impact

Potential silent publication of a unique index that should have failed, leaving durable uniqueness corruption / missing-or-wrong unique-index ownership for surviving rows.

Given that the trigger is a normal long-running online unique-index build plus concurrent DML under default GC settings, this is a severe correctness risk rather than a corner-case configuration bug.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.