[import] repeated handles can prematurely exhaust BoundedHandleSet's size limit
- Dominant language
- Go
- Stars
- 40.5k
- Forks
- 6.2k
- PR merge metrics
- PR metrics pending
Description
## Bug Report
### 1. Minimal reproduce step (Required)
This report is based on static analysis of current master. I did not run a TiDB-level reproduction.
A direct set-level regression test is enough to show the accounting defect:
```go
func TestBoundedHandleSetDuplicateDoesNotConsumeBudget(t *testing.T) {
var sharedSize atomic.Int64
handle := tidbkv.IntHandle(1)
delta := int64(len(handle.String())) + handleMapEntryShallowSize
set := NewBoundedHandleSet(zap.NewNop(), &sharedSize, 2*delta)
set.Add(handle)
set.Add(handle)
require.Len(t, set.handles, 1)
require.Equal(t, delta, sharedSize.Load())
require.False(t, set.BoundExceeded())
}
```
With the audited code, the map still has one entry, but `sharedSize` is `2 * delta` and `BoundExceeded()` is true.
A reachable `IMPORT INTO` shape is a unique multi-valued index where one row conflicts through two array elements:
```sql
CREATE TABLE t (
pk BIGINT PRIMARY KEY CLUSTERED,
a JSON NOT NULL,
UNIQUE KEY uk_a ((CAST(a->'$' AS UNSIGNED ARRAY)))
);
```
```text
1,"[1000,2000]"
2,"[1000]"
3,"[2000]"
```
Run a global-sort `IMPORT INTO` from supported external storage with `on_duplicate_key='capture'`. If the two index entries for `pk=1` are handled in different 256-handle buffers by the same worker, `Collector.HandleEncodedRow` calls `Add(1)` twice on the same worker-local set. A deterministic regression can reduce `BufferedHandleLimit`, enable `forceHandleConflictsBySingleThread`, and place the two occurrences in separate buffers.
### 2. What did you expect to see? (Required)
Adding an identity already present in a `BoundedHandleSet` should not increase the retained-set size estimate. `BoundExceeded()` and the derived `TooManyConflictsFromIndex` metadata should become true only when the retained handle set actually reaches its memory budget.
Repeated occurrences of the same row should not, by themselves, prematurely disable post-process checksum verification.
### 3. What did you see instead (Required)
`BoundedHandleSet.Add` checks the bound, computes `delta`, increments the shared counter, and only then assigns `s.handles[hdlStr] = true`. It never checks whether `hdlStr` already exists. Repeated calls for one retained identity therefore charge the entry repeatedly even though map assignment overwrites the same entry.
Once the inflated counter reaches the limit, `collectConflictsStepExecutor.onFinished` sets `TooManyConflictsFromIndex`. The planner copies that flag into post-process metadata, and the post-process executor returns without comparing the local and remote checksums. Thus the flag can describe an exhausted bound even when the unique retained set is below it.
This does **not** by itself establish direct data corruption. In the multi-valued-index scenario above, processing the same row twice also duplicates its deleted-row checksum contribution (tracked separately in #69799). Skipping checksum verification can be protective once that checksum is already inaccurate, because verification could otherwise fail on the bad checksum. The independent defect reported here is inaccurate set-bound accounting and the resulting premature verification-disable signal; fixing it must account for the separate checksum-deduplication bug rather than simply forcing verification on.
Static evidence:
- [`row_handle.go:81-100`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/row_handle.go#L81-L100) increments the counter before overwriting the map entry; [`row_handle.go:123-127`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/row_handle.go#L123-L127) derives the bound state only from that counter.
- [`collector.go:154-174`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/collector.go#L154-L174) calls `Add` for every index-conflict row delivered to the collector.
- [`collect_conflicts.go:136-153`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/collect_conflicts.go#L136-L153) writes the inflated bound state into `TooManyConflictsFromIndex`; [`planner.go:315-344`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/planner.go#L315-L344) propagates it to post-process metadata.
- [`subtask_executor.go:125-155`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/subtask_executor.go#L125-L155) skips checksum verification when the flag is true.
The existing [`TestBoundedHandleSet`](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/row_handle_test.go#L39-L70) only adds distinct handles. It has no regression asserting that re-adding the same handle leaves the shared size and bound state unchanged.
### 4. What is your TiDB version? (Required)
Current master audited at commit `59f6e85cd01756d53f799c33afba0a185d956d3f`.
No runtime `SELECT tidb_version()` output is available because this report is based on static analysis.
Contributor guide
Research direction
Start with pkg/dxf/importinto/conflictedkv/row_handle.go and its existing TestBoundedHandleSet in row_handle_test.go. Trace how BoundedHandleSet.Add updates handles and sharedSize, then run the focused conflictedkv tests. Done means re-adding a handle keeps one map entry, charges its size once, and does not prematurely set BoundExceeded; keep the separate checksum issue in #69799 in scope.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 74/100