pingcap / pingcap/tidb

statistics: clarify StatsCache.Put vs Update and the result-check asymmetry

Open
#68,536 1 comment 0 reactions 0 assignees View on GitHub
component/statistics sig/planner type/enhancement
Dominant language
Go
Stars
40.5k
Forks
6.2k
PR merge metrics
PR metrics pending

Description

## Enhancement

The stats cache currently has two write paths with different semantics:

- `StatsCacheImpl.Put` is used by the init-stats build path.
- `StatsCacheImpl.UpdateStatsCache` is used by normal stats cache updates.

The differences are subtle and are not documented. This issue tracks clarifying the intended contract of each path and, if possible, consolidating them after the non-quota stats-cache path is removed in #68528.

### Current behavior

`StatsCacheImpl.Put` -> `StatsCache.put` (`pkg/statistics/handle/cache/statscacheinner.go`):

- updates one table at a time;
- updates the cache in place;
- retries when the underlying cache reports that the write was not admitted;
- always advances `maxTblStatsVer`.

`StatsCacheImpl.UpdateStatsCache` (`pkg/statistics/handle/cache/statscache.go`):

- applies a batch of updated and deleted table stats;
- uses in-place `Update` when stats-cache quota is enabled;
- uses copy-on-write `CopyAndUpdate` when the non-quota path is used;
- respects `SkipMoveForward`.

### Questions to clarify

1. Is `Put` intentionally separate because init-stats needs in-place writes plus retry behavior?
2. After #68528 removes the non-quota path, can `Put` be folded into `UpdateStatsCache` or replaced by a clearer internal helper?
3. Should the code document why `Put` retries failed admissions while `Update` does not?

### Why the retry behavior is confusing

`putCache` checks the boolean returned by `sc.c.Put`, and `StatsCache.put` retries with backoff.

`Update` calls `sc.c.Put` once and ignores the return value.

At first glance, this looks inconsistent. The likely reason is that `LFU.Put` writes to the authoritative `resultKeySet` before calling Ristretto `Set`, and `Get` can fall back to `resultKeySet`. Therefore, a failed Ristretto admission does not lose table stats. The retry in `Put` mainly improves primary-cache admission during bulk init-stats loading, while the normal update path does not need that stronger admission behavior.

If this interpretation is correct, please document it near the two paths. If not, the two write paths should be made consistent.

### Minor follow-up

`LFU.Put` calls `addCost` before the Ristretto write is guaranteed to be admitted. If a write is rejected and no evict/exit callback fires, the tracked cache cost may drift upward slightly. This should be confirmed or fixed separately.

Related: #68528

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.