pingcap / pingcap/tidb

[import] conflict traffic metering miscounts cache hits and API V2 key prefixes

Open
#69,807 0 comments 0 reactions 0 assignees View on GitHub
component/DXF component/import severity/minor type/bug
Dominant language
Go
Stars
40.5k
Forks
6.2k
PR merge metrics
PR metrics pending

Description

## Bug Report

Conflict-resolution cluster traffic metering is inaccurate in two independent ways:

1. `LazyRefreshedSnapshot.BatchGet` adds `len(key) + len(value)` for every returned entry, including entries that client-go serves entirely from the snapshot cache without a TiKV RPC.
2. In NextGen/API V2, client-go exposes logical keys after codec decoding, while TiKV receives physical keys with a four-byte keyspace prefix. Conflict read and delete accounting uses the logical lengths and therefore omits four bytes per key. Classic/API V1 does not have this prefix discrepancy.

This is metering correctness only. It does not corrupt SQL rows, indexes, or conflict resolution results. `TrafficRecorder` is explicitly documented as best-effort, so this report is about making that best-effort estimate track actual cluster traffic more closely.

### 1. Minimal reproduce step (Required)

#### Test A: a repeated `BatchGet` is a cache hit but still grows the recorder

Add a deterministic unit test around `LazyRefreshedSnapshot` using one existing key `K` with value `V`:

1. Create one `LazyRefreshedSnapshot` with a counting `TrafficRecorder`.
2. Call `BatchGet(ctx, []Key{K})` once. This reads the entry and populates client-go's snapshot cache.
3. Record the traffic counter, install/count a client-go RPC interceptor, and call `BatchGet` again with the same key before the 15-second snapshot refresh interval expires.
4. Assert that the second call returns `K -> V`, issues zero `CmdBatchGet` RPCs, and inspect the recorder delta.

client-go's pinned `KVSnapshot.BatchGetWithTier` removes cache hits from the keys sent to the region/RPC path and returns immediately when no keys remain. TiDB nevertheless iterates over the returned map and records it again.

**Expected:** the second call's cluster-read byte delta is `0`, because the value is served from the local snapshot cache and no cluster read occurs.

**Actual at the audited HEAD:** the second call's cluster-read byte delta is `len(K) + len(V)` even though the intercepted `CmdBatchGet` RPC count is `0`. Repeating the call repeats the overcount.

#### Test B: API V2 physical-key lengths include four bytes that the recorder cannot see

Add a separate deterministic NextGen/API V2 test using a known logical key `K`, value `V`, and a client-go V2 transaction codec for a fixed keyspace ID:

1. Assert that `len(codec.EncodeKey(K)) == len(K) + 4`.
2. Read `K` through `LazyRefreshedSnapshot.BatchGet`. The response map key exposed after client-go response decoding is the logical `K`; compare the recorded bytes with the physical encoded-key length.
3. Delete the same logical key through `Deleter`. Compare the recorded write bytes with the V2-encoded delete-mutation key length.
4. Run the length assertions separately from Test A so the cache-hit result cannot affect this case.

**Expected in NextGen/API V2:** a returned pair contributes `len(codec.EncodeKey(K)) + len(V)`, or `len(K) + 4 + len(V)`, to cluster-read bytes. A delete contributes `len(codec.EncodeKey(K))`, or `len(K) + 4`, to cluster-write bytes.

**Actual at the audited HEAD:** the read recorder adds only `len(K) + len(V)`, and the delete recorder adds only `len(K)`. Both undercount by four bytes for every physical key. Classic/API V1 is unaffected by this prefix-specific case.

Relevant code and pinned dependency behavior:

- [`LazyRefreshedSnapshot.BatchGet` counts every returned logical entry](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/handler.go#L356-L377).
- [`Deleter` counts logical delete-key lengths before the transaction writes them](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/deleter.go#L151-L178).
- [`TrafficRecorder` describes this traffic as best-effort](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/importinto/conflictedkv/handler.go#L49-L55), and the concrete recorder stores the totals as cluster traffic ([implementation](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/pkg/dxf/framework/metering/recorder.go#L34-L42)).
- The audited [`go.mod` pins client-go](https://github.com/pingcap/tidb/blob/59f6e85cd01756d53f799c33afba0a185d956d3f/go.mod#L125) to `v2.0.8-0.20260708122311-01bd8f99f4da`.
- At that client-go revision, [`BatchGetWithTier` returns cached values without entering the RPC path when all keys hit](https://github.com/tikv/client-go/blob/01bd8f99f4da/txnkv/txnsnapshot/snapshot.go#L262-L291), and [`UpdateSnapshotCache` stores results for later calls](https://github.com/tikv/client-go/blob/01bd8f99f4da/txnkv/txnsnapshot/snapshot.go#L1133-L1152).
- Its API V2 codec defines a [four-byte keyspace prefix](https://github.com/tikv/client-go/blob/01bd8f99f4da/internal/apicodec/codec_v2.go#L26-L31), encodes physical keys by prepending it, and removes it when decoding ([codec methods](https://github.com/tikv/client-go/blob/01bd8f99f4da/internal/apicodec/codec_v2.go#L799-L820)).

Static-audit disclaimer: this report was derived from the code paths above. The proposed regression tests have not been added or executed as part of this audit, and no end-to-end billing impact was measured.

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

Conflict-handling cluster traffic should count bytes attributable to actual TiKV reads/writes: cached `BatchGet` results should not add cluster-read bytes again, and API V2 accounting should use the physical encoded-key length including the four-byte keyspace prefix.

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

Repeated reads can increase `clusterReadBytes` without a corresponding RPC, while NextGen/API V2 reads and deletes undercount four bytes for each physical key. Depending on access patterns, the two read errors can partially offset or dominate one another; they should be tested and fixed independently.

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

Audited source HEAD: `59f6e85cd01756d53f799c33afba0a185d956d3f`.

Pinned client-go: `github.com/tikv/client-go/v2 v2.0.8-0.20260708122311-01bd8f99f4da`.

The static evidence does not establish an affected released TiDB version, so no `affects-*` label is requested.

Contributor guide

Open the contributing guide

Research direction

Start with pkg/dxf/importinto/conflictedkv/handler.go and deleter.go, then inspect the pinned client-go snapshot and API V2 codec behavior referenced in the report. Add separate deterministic tests for repeated cached BatchGet calls and NextGen/API V2 key lengths. Done means cache hits add zero cluster-read bytes and V2 read/delete accounting matches physical encoded-key lengths, without changing conflict results.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.