ddl: TRUNCATE PARTITION global index cleanup deletes multi-valued index entries owned by other partitions
- Dominant language
- Go
- Stars
- 40.5k
- Forks
- 6.2k
- PR merge metrics
- PR metrics pending
Description
## Bug Report
### 1. Minimal reproduce step (Required)
Requires a **`GLOBAL` + `UNIQUE` + multi-valued** index on a partitioned table, plus a concurrent write that takes over an element key still owned by a partition being truncated.
```sql
CREATE TABLE t (
p INT PRIMARY KEY,
j JSON,
UNIQUE KEY m ((CAST(j AS UNSIGNED ARRAY))) GLOBAL
) PARTITION BY RANGE (p) (
PARTITION p0 VALUES LESS THAN (10),
PARTITION p1 VALUES LESS THAN (20),
PARTITION p2 VALUES LESS THAN MAXVALUE
);
INSERT INTO t VALUES (1, '[100]'); -- p0
INSERT INTO t VALUES (11, '[7]'); -- p1, owns element 7
```
Then run `ALTER TABLE t TRUNCATE PARTITION p0, p1` while another session inserts a row that takes over element `7`. The window is `StateDeleteReorganization`, between the cleanup of `p0` and the cleanup of `p1`; it is made deterministic with the `ddl/mockDMLExecution` failpoint:
```go
func TestTruncatePartitionGlobalMVIndexCleanup(t *testing.T) {
store := testkit.CreateMockStore(t)
tk := testkit.NewTestKit(t, store)
tk.MustExec("use test")
tk.MustExec(`CREATE TABLE t (
p INT PRIMARY KEY,
j JSON,
UNIQUE KEY m ((CAST(j AS UNSIGNED ARRAY))) GLOBAL
) PARTITION BY RANGE (p) (
PARTITION p0 VALUES LESS THAN (10),
PARTITION p1 VALUES LESS THAN (20),
PARTITION p2 VALUES LESS THAN MAXVALUE
)`)
tk.MustExec("INSERT INTO t VALUES (1, '[100]')")
tk.MustExec("INSERT INTO t VALUES (11, '[7]')")
tk1 := testkit.NewTestKit(t, store)
tk1.MustExec("use test")
ddl.MockDMLExecution = func() {
// Take over element 7 before the p1 cleanup runs.
tk1.MustExec("INSERT INTO t VALUES (21, '[7]')")
}
require.NoError(t, failpoint.Enable(
"github.com/pingcap/tidb/pkg/ddl/mockDMLExecution", "1*return(true)->return(false)"))
tk.MustExec("ALTER TABLE t TRUNCATE PARTITION p0, p1")
require.NoError(t, failpoint.Disable("github.com/pingcap/tidb/pkg/ddl/mockDMLExecution"))
ddl.MockDMLExecution = nil
// Every assertion below fails before the fix.
require.NoError(t, tk.ExecToErr("ADMIN CHECK TABLE t"))
require.Error(t, tk.ExecToErr("INSERT INTO t VALUES (22, '[7]')"))
tk.MustQuery("SELECT p FROM t WHERE 7 MEMBER OF (j)").Check(testkit.Rows("21"))
}
```
This is not a failpoint-only scenario: in production any write into an unaffected/new partition that reuses an element key owned by a dropping partition during `StateDeleteReorganization` is affected. The failpoint only removes the timing uncertainty.
### 2. What did you expect to see? (Required)
After the DDL completes:
* the concurrently inserted row keeps its global index entry and `ADMIN CHECK TABLE` is clean;
* the `UNIQUE` constraint still holds, so inserting another `[7]` fails with a duplicate-key error;
* queries that use the index (`7 MEMBER OF (j)`) return the row.
This is the guarantee the cleanup code documents for itself (`#65418`): *"uses `BatchGet` to find current index entries, and only locks and deletes when the entry value decodes to `kv.PartitionHandle` with `PartitionID == scanned partition ID`, avoiding accidental deletion of entries written by new/other partitions."*
### 3. What did you see instead? (Required)
```
ADMIN CHECK TABLE t
-> [admin:8223]data inconsistency in table: t, index: m, handle: 21,
index-values:"" != record-values:"handle: 21, values: [KindMysqlJSON [7]]"
INSERT INTO t VALUES (22, '[7]')
-> err= -- UNIQUE no longer enforced
SELECT p FROM t WHERE 7 MEMBER OF (j)
-> [] -- index plan silently misses the live row
SELECT p FROM t IGNORE INDEX(m) WHERE 7 MEMBER OF (j)
-> [[21]] -- the row is really there
```
`EXPLAIN` confirms the query does use the damaged index:
```
Projection_4
└─IndexMerge_11 type: union
└─IndexRangeScan_9(Build) table:t, index:m(cast(`j` as unsigned array)) range:[7,7]
```
The damage then spreads through ordinary DML: deleting row `21` (whose entry is already missing) removes the entry now owned by row `22`, because a public index deletes the computed key unconditionally (`okToDelete` is only active when `BackfillState != BackfillStateInapplicable`). `ADMIN CHECK` then reports handle `22`.
### Root cause
`cleanUpIndexWorker.BackfillData` (`pkg/ddl/index.go`, around line 3984) builds the keys used for `BatchGet`, the partition-owner check and `LockKeys` with `GenIndexKey`:
```go
key, distinct, err := w.indexes[i%n].GenIndexKey(ec, loc, idxRecord.vals, idxRecord.handle, nil)
```
`pkg/table/index.go` documents that this API is single-key only:
> `GenIndexKey` generates an index key. **If the index is a multi-valued index, use `GenIndexKVIter` instead.**
For a multi-valued index `idxRecord.vals` is the whole JSON array, so `GenIndexKey` returns the encoding of the array, while the index actually stores **one entry per distinct element**. `BatchGetValue(globalIndexKeys)` therefore never finds anything, `found` is empty, and both the owner check and `LockKeys` are skipped. `Delete` is then called unconditionally; it expands the array and deletes every element key, including one a concurrent write has just taken over.
Two distinct defects in one place:
1. the owner guard is dead code for multi-valued indexes — the check key is never a stored key;
2. the delete decision is made once per row and applied to all elements, so even a correct whole-row guard could not preserve a single taken-over element while removing the rest of a row such as `'[7, 8]'` where only `7` was taken over.
`allowOverwriteOfOldGlobalIndex` (`pkg/table/tables/index.go`, added by #55831 for #55819) is what lets the concurrent writer take over the key during `StateDeleteReorganization`. `DROP PARTITION` does not enable it, so it rejects the same concurrent write with a duplicate-key error instead — safe but unnecessarily blocking, which matches the existing `// TODO: Also do the same for DROP PARTITION`.
### 4. What is your TiDB version? (Required)
```
master @ 02e5d7b3ec
(reproduced with pkg/ddl + testkit.CreateMockStore; also reproduced on v9.0.0-beta.2.pre-2268-g81b935a8ba)
```
No TiKV cluster is needed: `testkit.CreateMockStore` reproduces it, and `ADMIN CHECK TABLE` detects the corruption.
Contributor guide
Research direction
Start in pkg/ddl/index.go at cleanUpIndexWorker.BackfillData and trace how global index keys are generated, fetched, checked for partition ownership, locked, and deleted. Read the GenIndexKey and GenIndexKVIter documentation in pkg/table/index.go, then reproduce the supplied test with testkit.CreateMockStore and ddl/mockDMLExecution. Done means the concurrent row keeps its index entry, ADMIN CHECK TABLE is clean, duplicate insertion fails, and indexed lookup returns the row.
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