statistics: remove historical stats feature#69889
Open
0xPoe wants to merge 5 commits into
Open
Conversation
ti-chi-bot
Bot
added
release-note
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR removes historical statistics generation and snapshot dumping across TiDB. It simplifies statistics storage APIs, plan replayer behavior, HTTP handlers, compatibility variables, metrics, dashboards, documentation, bootstrap wiring, and related tests while retaining cleanup of legacy history rows. ChangesHistorical statistics removal
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…residue Follow-ups to the historical stats feature removal: - Fix ClearOutdatedHistoryStats returning after its first 50-row batch and deriving the batch count from the wrong table: count and drain mysql.stats_meta_history and mysql.stats_history independently, and add regression tests for both kept legacy drain paths. - Warn when serving pre-upgrade capture_replayer_*.zip files, which no longer get download-time stats injection. - Delete residue orphaned by the removal: unused plan replayer test helpers, the stale panic-when-record-historical-stats-meta failpoint enable, the Snapshot route constant, the removed endpoint's entry in docs/tidb_http_api.md, the source parameter on the stats save APIs and the StatsMetaHistorySource* constants, write-only SessionVars.EnableHistoricalStats and subscriber.statsCache, dead uint64 version returns, and the EnableHistoricalStatsForCapture atomic and hooks. - Make tidb_enable_historical_stats and tidb_enable_historical_stats_for_capture append a removed-feature warning when set to ON, and correct the tidb_historical_stats_duration comment, which still tunes the legacy drain retention.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/statistics/handle/storage/stats_read_writer.go`:
- Line 119: Update the error return in the stats metadata version update path to
wrap and preserve the original err, using context-neutral wording such as a
general stats-save failure rather than referencing analyze-result saving. Keep
the actionable retry guidance while ensuring the underlying cause remains
available to callers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f544f1e2-19d2-4430-a3be-76acba5fec01
📒 Files selected for processing (78)
br/pkg/restore/snap_client/pipeline_items.gobr/pkg/restore/snap_client/pipeline_items_test.godocs/tidb_http_api.mdpkg/domain/BUILD.bazelpkg/domain/domain.gopkg/domain/extract.gopkg/domain/historical_stats.gopkg/domain/metrics/metrics.gopkg/domain/plan_replayer.gopkg/domain/plan_replayer_dump.gopkg/domain/plan_replayer_test.gopkg/executor/BUILD.bazelpkg/executor/analyze.gopkg/executor/analyze_global_stats.gopkg/executor/analyze_worker.gopkg/executor/builder.gopkg/executor/historical_stats_test.gopkg/executor/plan_replayer.gopkg/executor/test/loaddatatest/load_data_test.gopkg/executor/test/planreplayer/plan_replayer_test.gopkg/metrics/grafana/tidb.jsonpkg/metrics/metrics.gopkg/metrics/nextgengrafana/tidb_with_keyspace_name.jsonpkg/metrics/nextgengrafana/tidb_worker.jsonpkg/metrics/stats.gopkg/parser/ast/misc.gopkg/parser/ast/misc_test.gopkg/parser/parser.gopkg/parser/parser.ypkg/planner/core/BUILD.bazelpkg/planner/core/common_plans.gopkg/planner/core/planbuilder.gopkg/server/BUILD.bazelpkg/server/handler/optimizor/BUILD.bazelpkg/server/handler/optimizor/optimize_trace.gopkg/server/handler/optimizor/plan_replayer.gopkg/server/handler/optimizor/plan_replayer_test.gopkg/server/handler/optimizor/statistics_handler.gopkg/server/handler/optimizor/statistics_handler_test.gopkg/server/handler/util.gopkg/server/http_handler.gopkg/server/http_status.gopkg/session/session.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/sysvar_test.gopkg/statistics/BUILD.bazelpkg/statistics/handle/BUILD.bazelpkg/statistics/handle/ddl/BUILD.bazelpkg/statistics/handle/ddl/ddl.gopkg/statistics/handle/ddl/subscriber.gopkg/statistics/handle/globalstats/global_stats.gopkg/statistics/handle/handle.gopkg/statistics/handle/handletest/BUILD.bazelpkg/statistics/handle/handletest/handle_test.gopkg/statistics/handle/handletest/lockstats/BUILD.bazelpkg/statistics/handle/handletest/lockstats/lock_table_stats_test.gopkg/statistics/handle/history/BUILD.bazelpkg/statistics/handle/history/history_stats.gopkg/statistics/handle/metrics/metrics.gopkg/statistics/handle/storage/BUILD.bazelpkg/statistics/handle/storage/dump_test.gopkg/statistics/handle/storage/gc.gopkg/statistics/handle/storage/gc_test.gopkg/statistics/handle/storage/json.gopkg/statistics/handle/storage/save.gopkg/statistics/handle/storage/stats_read_writer.gopkg/statistics/handle/storage/stats_read_writer_test.gopkg/statistics/handle/types/interfaces.gopkg/statistics/handle/updatetest/update_test.gopkg/statistics/handle/usage/BUILD.bazelpkg/statistics/handle/usage/session_stats_collect.gopkg/statistics/handle/util/util.gopkg/statistics/integration_test.gopkg/statistics/util/json_objects.gopkg/testkit/mockstore.gopkg/util/replayer/replayer.go
💤 Files with no reviewable changes (37)
- pkg/server/BUILD.bazel
- pkg/metrics/metrics.go
- pkg/server/handler/optimizor/optimize_trace.go
- pkg/planner/core/BUILD.bazel
- pkg/domain/BUILD.bazel
- pkg/domain/historical_stats.go
- pkg/server/handler/util.go
- pkg/statistics/handle/handle.go
- pkg/executor/test/loaddatatest/load_data_test.go
- pkg/statistics/handle/history/BUILD.bazel
- pkg/statistics/handle/BUILD.bazel
- pkg/statistics/handle/handletest/lockstats/BUILD.bazel
- pkg/executor/analyze_global_stats.go
- pkg/metrics/grafana/tidb.json
- pkg/metrics/stats.go
- pkg/statistics/handle/updatetest/update_test.go
- pkg/statistics/handle/handletest/handle_test.go
- pkg/server/handler/optimizor/statistics_handler.go
- pkg/executor/test/planreplayer/plan_replayer_test.go
- pkg/executor/BUILD.bazel
- pkg/statistics/handle/ddl/BUILD.bazel
- pkg/testkit/mockstore.go
- pkg/statistics/handle/globalstats/global_stats.go
- pkg/metrics/nextgengrafana/tidb_worker.json
- pkg/statistics/handle/usage/BUILD.bazel
- pkg/sessionctx/variable/session.go
- pkg/statistics/handle/handletest/lockstats/lock_table_stats_test.go
- pkg/domain/metrics/metrics.go
- pkg/metrics/nextgengrafana/tidb_with_keyspace_name.json
- pkg/planner/core/planbuilder.go
- pkg/statistics/handle/history/history_stats.go
- pkg/server/http_status.go
- pkg/statistics/handle/storage/dump_test.go
- pkg/executor/historical_stats_test.go
- pkg/server/handler/optimizor/plan_replayer_test.go
- pkg/executor/analyze.go
- pkg/statistics/handle/storage/json.go
… as a no-op Keep accepting `PLAN REPLAYER DUMP ... WITH STATS AS OF TIMESTAMP ...` for compatibility after the historical stats feature removal: the clause is ignored, the dump contains current statistics, and a warning is reported. The warning is appended after the dump finishes because the internal SQL executed while dumping resets the session's statement context, so a plan-building-time warning would not survive to SHOW WARNINGS.
0xPoe
force-pushed
the
poe/remove-historical-stats-master
branch
from
8873b70 to
0ce1485
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69889 +/- ##
================================================
- Coverage 76.3206% 75.6195% -0.7012%
================================================
Files 2041 2085 +44
Lines 559973 581826 +21853
================================================
+ Hits 427375 439974 +12599
- Misses 131697 139707 +8010
- Partials 901 2145 +1244
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
The historical stats removal left the DDL subscriber helper methods and buildPlanReplayer with receivers that are no longer referenced, which fails bazel nogo's revive unused-receiver check; use anonymous receivers. Also sync pkg/bindinfo/tests shard_count with the gazelle output required by check-bazel-prepare.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #52826
Problem Summary: Recording historical statistics keeps large internal INSERT statements alive in the domain session pool and can use excessive memory. The feature is off by default, rarely used, and not worth its memory and maintenance cost, so this PR removes it.
What changed and how does it work?
RecordHistoricalStatsMeta/RecordHistoricalStatsToStoragewrite paths, the domain worker, the/stats/dump/{db}/{table}/{snapshot}HTTP API, related metrics/dashboard panels, plumbing, and tests.PLAN REPLAYER DUMP ... WITH STATS AS OF TIMESTAMP ...is still accepted — the clause is ignored, the dump contains current statistics, and a warning is reported.tidb_enable_historical_statsandtidb_enable_historical_stats_for_capturestay registered as no-op variables that report a warning when set toON;mysql.stats_history/mysql.stats_meta_historyare kept so upgraded clusters keep working.tidb_historical_stats_duration. This also fixes a bug inClearOutdatedHistoryStatsthat deleted at most one 50-row batch ofmysql.stats_historyper GC round (and derived the batch count from the wrong table), which would have left legacy rows behind forever; regression tests cover both drain paths.capture_replayer_*.zipbundles, since they contain no statistics and no longer get download-time stats injection.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
WITH STATS AS OF TIMESTAMPis accepted but ignored, and a warning is shown.