tikv / tikv/pd

PD Follower Staleness Gating

Open
#11,115 6 comments 0 reactions 0 assignees View on GitHub
contribution first-time-contributor type/feature-request
Dominant language
Go
Stars
1.2k
Forks
783
Avg merge
5d 21h
Merged PRs (30d)
36

Description

# PD follower staleness gating for `pd_enable_follower_handle_region`

## Component / feature

`pd_enable_follower_handle_region` (pd_EFHR): PD follower handling of region-metadata RPCs. Default `OFF`.

Files: `tidb-pd` `pkg/syncer/{client,server,history_buffer}.go`, `server/grpc_service.go`, `client/{client.go,opt/option.go}`; `tidb-db` `sessionctx/variable/*`, `domain/domain_sysvars.go`, `store/tikv/region_cache.go`.

## Problem statement + impact

**No staleness gate on followers.** With pd_EFHR on, the only protection is the TiDB/follower epoch comparison, and it is bypassable. A lagging follower serves stale region info, TiDB carries it to TiKV, TiKV rejects it, and the re-lookup lands back on the leader. Correlated staleness makes this a synchronized burst from many TiDB nodes at once, which is the exact pressure pd_EFHR was meant to remove.

## Existing behavior + limitation

TiDB round-robins across leader and followers. On a follower hit it retries against the leader if `follower_epoch < tidb_epoch`, else accepts. Followers gate on `IsRunning()`; `false` returns `regionNotFound()`, which triggers client `NeedRetry`, a leader retry, and a 10s circuit break on that follower.

1. **Epoch check has three holes:** a cold TiDB cache has no floor to compare against; TTL expiry with a stale local floor; a region leader change does not bump the epoch.
2. **TiKV's correction is not always usable.** It normally returns `EpochNotMatch` plus leader store ID, or `NotLeader` plus full region descriptors, but has nothing usable when there is no leader, a hibernating region was just woken, a peer is Raft-isolated, or a peer is mid-removal. The client must then return to PD.
3. **Invalidation can launder a stale answer into a fresh TTL.** `InvalidateCachedRegionWithReason` flips `ttl = -1` but not `mu.latestVersions[id]`. The reload permits a follower; a follower that is also behind returns the epoch TiDB already had, so `oldVer.GetVer() > newVer.GetVer()` is false, the answer is accepted, `newRegion` assigns a fresh TTL, and the leader-pinned retry in `findRegionByKey` never fires because it is gated on a detected regression.
4. **`IsRunning()` is too coarse.** It is binary and initial-sync-scoped, so a follower that synced then stalled still reports healthy. Existing `sync_index` gauges persist on a 100-record cadence: fine for dashboards, too stale for per-request gating.

## Proposals for path forward

Two proposals, complementary rather than either/or. P0 is the fundamental fix: it stops a stale follower from answering at all. P1 helps on top of it: it gives the client a path to retry against the leader once it knows a follower has already been tried and proven wrong for that region, which catches what slips under P0's thresholds.

### p0: Lag detection + reuse of existing PD retry handling

Add two lightweight staleness signals to the follower's health gate. Both feed the existing `IsRunning() == false` to `regionNotFound()` to `NeedRetry` path, so there is no new error type, no new response shape, and no client-side change.

**`Lag()` = `leaderHeadIndex - followerCurrentIndex`.** Followers cannot query the leader's index directly, so derive it from `SyncRegionResponse.StartIndex` on the sync stream. `StartIndex` is overloaded (batch start on updates, live tip on keepalives), so normalize:

```go
s.leaderHeadIndex.Store(resp.GetStartIndex() + uint64(len(resp.GetRegions())))
```

The follower's index is `historyBuffer.index` via `getNextIndex()`, incremented by `record()` per region off the stream (`client.go:236-240`), tracking stream processing rather than disk writes. Fires when `Lag() > indexThreshold`.

**`TimeSinceLastSync()`:** wall-clock field stamped `lastSyncTime = time.Now()` when the receive-and-apply loop finishes a message. No disk I/O, no lock contention, no allocation. Fires when `> timeThreshold`.

Both are stored at receipt, before the apply loop (after the mismatch check at ~`client.go:190`, before `:197`), so the gate reflects current leader state. Fields follow the existing lock-free atomic pattern of `streamingRunning` (`server.go:87-88`).

Both signals are needed: `maxSyncRegionBatchSize` is 100, so a follower stalled mid-apply shows at most ~100 lag, making `TimeSinceLastSync()` the primary gate for wedges the index metric cannot see.

`indexThreshold` and `timeThreshold` become tunable knobs; defaults left to PingCAP.

**Known limits, documented rather than fixed here:** `Lag()` is one global scalar over the whole region-change stream, so it can overshoot for a given region; the history buffer is a 10,000-entry ring (`server.go:48`) and a follower further behind may miss a full resync in `syncHistoryRegion` (`server.go:303-304`); the index survives restarts but buffered records do not (`record()` flushes every 100, `history_buffer.go:97-101`), so a restarted follower can report a nonzero index with no local records.

#### Implementation cost

Roughly 100 lines of production code plus ~200 of tests: syncer atomics and accessors in `client.go` / `server.go` / `history_buffer.go`, a lag/staleness clause added to the gating branch in all four region RPCs in `grpc_service.go`, and two threshold knobs. No new concurrency idioms, error types, wire-format changes, or client changes, so blast radius is confined to the follower-serve decision.

Behavior change: a lagging or stalled follower returns `regionNotFound()` where it previously served, costing one extra round-trip plus a 10s circuit break, both already-existing behaviors on the not-found path.

#### Test plan

**Unit:** `Lag()` normalization across both `StartIndex` shapes; `TimeSinceLastSync()` stamping point relative to the apply loop; threshold boundaries; restarted follower with a nonzero index and empty buffer must not report healthy.

**Integration / fault injection:** throttle the sync stream to one follower and assert `regionNotFound()`, leader retry, circuit break, and a correct client answer; stall the apply loop while the stream keeps delivering to confirm `TimeSinceLastSync()` catches what capped `Lag()` cannot; push a follower past the ring size and document behavior; drive a follower past threshold and confirm it refuses to serve, so neither Limitation 3 route is reachable from it.

**Load regression:** rerun the sysbench cold-start scenarios with the gate on to confirm leader offload is preserved and the gate does not over-trigger back to leader-only, errors stay at zero, follower uptake stays even, and TSO wait holds. Sweep `indexThreshold` against `timeThreshold` to inform defaults and measure false triggers.

**Sysvar:** existing `set_test.go` and `vars.test` still pass (global SET works, session SET disallowed, default `OFF`); new knobs propagate to the running PD client at runtime.

### p1: One-strike marker that pins the next PD reload to the leader

Mark a region when TiKV proves its cached data wrong, then omit `WithAllowFollowerHandle()` on the next reload of that region so the leader answers. Closes both routes in Limitation 3, where the existing escalation is gated on a detected epoch regression and never fires when a lagging follower returns the epoch TiDB already had.

**Mechanism.** One field on the cached `Region`, alongside the existing CAS'd `invalidReason`.

**Set sites (three):**

1. `OnRegionEpochNotMatch`, empty `CurrentRegions` (`region_cache.go:2546-2548`). TiKV rejected the entry with nothing to replace it, so the marker rides the existing CAS write.
2. `OnRegionEpochNotMatch`, equal-epoch branch (`:2570-2591`), set on the **new** `Region`, not the old one. Non-empty `CurrentRegions` means `needInvalidateOld = false` when `ctx.Region == region.VerID()`, so site 1 never runs, and the object from `newRegion()` (`:2576`) is installed by `insertRegionToCache` (`:2598`) carrying the same stale data, a fresh TTL, and an `invalidReason` of `Ok`. Set the marker on `region` inside that loop before it is appended to `newRegions`. This is the mainline case.
3. `replicaSelector.onNotLeader`, `leader == nil` (`replica_selector.go:487-491`), set on `s.region`. This is the branch leaderless `NotLeader` actually reaches, and it touches the cache nowhere today. The lookalike at `region_request.go:1477-1489` only runs when `s.replicaSelector == nil`, effectively never, since `getRPCContext` builds a selector lazily. Mark it too, but it is not load-bearing.

**Read sites:** `findRegionByKey` / `loadRegion` (`:1487-1497`), `scanRegions` (`:2180-2201`), `batchScanRegions` (`:2242-2270`). `searchCachedRegionByKey` (`:1489`) already returns the expired object, so this is a field read on something in scope rather than a second lookup.

**No clear site.** Reload paths build a fresh `Region` via `newRegion()` and replace the entry wholesale, so the zero-value marker gives one-shot behavior for free. Set site 2 is the deliberate exception, carrying the marker forward because its data is not actually new.

**Leader targeting** needs no new code: the client already dials its leader-tracking connection directly, falling back to follower-plus-forward only if it believes the leader is down.

**Flow:** a follower returns equal-epoch `CurrentRegions`; the equal-epoch branch marks the new object before insert; the next reload skips the follower; the leader answers; the marker lapses with the object it replaces. A fresh TTL then only ever attaches to a leader-confirmed epoch, and the loop ends in one extra round-trip instead of a full cache interval.

#### Implementation cost

70 to 90 lines of production code plus 150 to 180 of tests, all client-side: one `Region` field, three set clauses, a marker check at three load paths, and a range-overlap check for scans. No clear-site code, no PD-side change, no new config, no wire-format change, so it can ship independently. It does land in the region cache's hot, concurrency-sensitive path rather than in the syncer. Review attention belongs on set site 2 and the `onNotLeader` change, since neither is reachable by hooking the existing invalidate call.

Behavior change: one reload per proven-wrong region goes to the leader, now including the equal-epoch and selector-driven leaderless cases.

#### Test plan

**Unit:** marker set only for rejections with no usable correction, not ordinary TTL expiry; set on the newly-inserted object for equal-epoch `CurrentRegions`, asserted against what is in the cache map after the call; set through the selector-present `onNotLeader` path; absent on any ordinary fresh `Region`; concurrent invalidate and reload leaves the cache consistent; range rule pins a scan when one covered region is marked.

**Integration:** reproduce Limitation 3 with equal-epoch rather than empty `CurrentRegions`, and assert the reload skips the follower, the leader answers, no fresh TTL is written for the stale epoch, and the region is follower-eligible again on the following reload.

**Load regression:** rerun the sysbench cold-start scenarios; added leader traffic should stay proportional to the rejection rate without measurably eroding the pd_EFHR offload.

Contributor guide

Open the contributing guide

Research direction

First resolve whether the implementation should pursue P0 in tidb-pd/pkg/syncer and server/grpc_service.go, P1 in tidb-db/store/tikv/region_cache.go and replica_selector.go, or both. Read the cited syncer and region-cache entry points, then run the listed unit and integration tests. Done means stale followers refuse or bypass serving as specified, leader retry remains correct, and the sysbench and sysvar regressions pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend, distributed-systems
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.