pingcap / pingcap/tidb

BR: 2 passes pre-split causing restore performance regression

Open
#69,015 3 comments 0 reactions 0 assignees View on GitHub
component/br contribution severity/moderate type/bug type/regression
Dominant language
Go
Stars
40.5k
Forks
6.2k
PR merge metrics
PR metrics pending

Description

## Bug Report

Please answer these questions before submitting your issue. Thanks!

### 1. Minimal reproduce step (Required)

Run PiTR restore on the same test topology and similar data volume with the following two TiDB commits:

- Fast commit: `81ec977cb8bf97e0c9805dfc0be8ffcbd4b0bbeb`
- Slow commit: `e70762ebf8bc466aa251e893799a7d8e42b89a69`

Clinic links:

- Fast restore: https://clinic.pingcap.com.cn/portal/#/orgs/33/clusters/7645071090219078379?from=1780006732&to=1780013527
- Slow restore: https://clinic.pingcap.com.cn/portal/#/orgs/33/clusters/7647273150717798736?from=1780519436&to=1780534238

Both clusters have the same basic topology:

- 1 TiDB
- 6 TiKV
- 1 PD
- TiDB version: nightly/master build

The main BR-related change between the two commits is:

```text
72e854aac662020f67e280cc04a0964f066c7838 br: two-pass pre-split for PiTR log restore (#67310)
```

The slow commit adds a PiTR restore pre-split path:

```go
preSplitDone, preSplitErr := client.PreSplitRegions(ctx, rewriteRules, splitSize, splitKeys)
```

When `preSplitDone == true`, the log restore path skips the original per-batch split wrapper and only keeps checkpoint filtering:

```go
logFilesIterWithSplit, err = client.WrapLogFilesIterWithCheckpointFilter(...)
```

instead of the old path:

```go
logFilesIterWithSplit, err = client.WrapLogFilesIterWithSplitHelper(...)
```

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

PiTR restore performance should not regress after introducing two-pass pre-split.

After pre-split completes, BR should still feed TiKV import/download continuously. TiKV import download throughput and apply task rate should stay close to the previous commit for the same topology and similar data volume.

Expected behavior:

- TiKV import download should remain continuously active.
- `tikv_import_download_bytes_sum` throughput should be close to the fast commit.
- `tikv_import_download_bytes_count` should not drop significantly.
- Overall restore duration should not increase by around 2x when TiKV download/read latency itself is unchanged.

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

The restore on the slow commit took much longer:

```text
Fast restore duration: ~113 min
Slow restore duration: ~247 min
Regression: ~2.18x slower
```

The total restored/write volume is similar, but the slow restore has much lower effective TiKV import download throughput.

Key metrics:

```text
tikv_import_download_bytes_sum avg:
fast: ~357 MB/s
slow: ~206 MB/s
slow/fast: ~58%

tikv_import_download_bytes_count avg:
fast: ~1432 ops/s
slow: ~734 ops/s
slow/fast: ~51%

download read avg:
fast: ~6.47s
slow: ~6.53s

download read p95:
fast: ~15.5s
slow: ~17.6s

download >100MB/s active ratio:
fast: ~62%
slow: ~31%

tikv_import_ingest_bytes_sum avg:
fast: ~752 MB/s
slow: ~344 MB/s
slow/fast: ~46%
```

TiKV does not appear saturated in the slow case:

```text
TiKV CPU avg:
fast: ~25.7 cores
slow: ~17.4 cores

apply log p99:
fast: ~94 ms
slow: ~8 ms

request wait p99:
fast: ~1.9 ms
slow: ~0.07 ms
```

This suggests the bottleneck is not TiKV download latency, TiKV CPU, raft apply, or disk write latency. The slow case looks like TiKV import/download is underfed: each download is not much slower, but BR submits far fewer download/apply tasks and leaves TiKV idle more often.

Possible cause:

The new two-pass pre-split path changes the log restore file iterator path. When pre-split succeeds, BR skips `WrapLogFilesIterWithSplitHelper` and uses `WrapLogFilesIterWithCheckpointFilter`. The skipped wrapper is not only doing split; it is also part of the streaming restore pipeline before `RestoreKVFiles`. Removing it may change the cadence and region/batch shape seen by `ApplyKVFilesWithBatchMethod`, causing fewer `ApplyKVFile` requests and lower TiKV import download concurrency.

There is also a potential amplification factor in `PreSplitRegions`: it currently merges all DML files into the split helper, including files <= 1MB. The old per-batch `LogSplitStrategy.Accumulate` skipped files <= `SplitFileThresholdDefault` to avoid excessive split/scatter and BTreeMap overhead. The new pre-split path does not apply that threshold.

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

Fast commit:

```text
81ec977cb8bf97e0c9805dfc0be8ffcbd4b0bbeb
```

Slow commit:

```text
e70762ebf8bc466aa251e893799a7d8e42b89a69
```

Both are nightly/master builds according to Clinic metadata.

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.