pingcap / pingcap/tidb

[import] repeated handles can prematurely exhaust BoundedHandleSet's size limit

Open
#69,803 0 comments 0 reactions 0 assignees View on GitHub
component/DXF component/global-sort component/import found-by-ai severity/moderate type/bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.