statistics: deprecate enable-stats-cache-mem-quota and remove the non-quota stats cache path
- Dominant language
- Go
- Stars
- 40.5k
- Forks
- 6.2k
- PR merge metrics
- PR metrics pending
Description
`enable-stats-cache-mem-quota` defaults to `true`, but we still keep the old non-quota stats cache path.
When the flag is `true`, stats cache uses the `lfu` backend and updates the cache in place.
When the flag is `false`, it uses the old `mapcache` backend and updates the cache through `CopyAndUpdate`.
The non-quota path is already effectively legacy code. There is even a TODO saying this branch should be removed because quota will always be enabled.
Besides being extra maintenance burden, this old path also has a correctness issue: `CopyAndUpdate` does not respect `UpdateOptions.SkipMoveForward`, while `Update` does.
This matters for the targeted stats refresh after `ANALYZE`. That refresh sets `SkipMoveForward = true`, so it should not move the cache watermark forward. But if `enable-stats-cache-mem-quota = false`, the COW path may still advance `maxTblStatsVer`. After that, the next incremental `stats_meta` scan may skip some delta rows from other tables, leaving their stats stale until they are analyzed again.
The default config is not affected because quota is enabled by default.
I think we should deprecate this flag and remove the legacy path instead of fixing dead code.
Proposed changes:
* Keep parsing `enable-stats-cache-mem-quota`, but always treat it as `true`.
* Log a warning if users set it to `false`.
* Remove the `mapcache` branch in `NewStatsCache`.
* Remove `CopyAndUpdate` and the old branch in `UpdateStatsCache`.
* Remove the `internal/mapcache` package if there are no other users.
* Update tests and benchmarks that still set this flag to `false`.
Contributor guide
Assessment
This issue has not been assessed yet.