Repository navigation
statistics: skip stale stats meta during initialization - #69116
Conversation
ti-chi-bot
Bot
added
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR updates statistics initialization to skip stale metadata for dropped tables. ChangesSchema-aware stats metadata filtering
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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 |
ti-chi-bot
Bot
added
size/L
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
| defer store.Close() | ||
| se := session.CreateSessionAndSetID(t, store) | ||
| session.MustExec(t, se, "use test") | ||
| session.MustExec(t, se, "set @@tidb_partition_prune_mode='static'") |
There was a problem hiding this comment.
Why would we still use static mode here? It is about to be decapitated.
| tk.MustExec("analyze table t all columns;") | ||
| tk.MustExec("drop table t") | ||
| h := dom.StatsHandle() | ||
| tk.MustExec("set @@tidb_partition_prune_mode='static'") |
| } | ||
| for row := iter.Begin(); row != iter.End(); row = iter.Next() { | ||
| physicalID = row.GetInt64(1) | ||
| maxPhysicalID = max(physicalID, maxPhysicalID) |
There was a problem hiding this comment.
maxPhysicalID is updated before the stale-ID skip. That means a dropped table with a high physical ID can still inflate full-init histogram/TopN/bucket paging ranges even though the PR skips its meta entry.
0xPoe
added
the
needs-cherry-pick-release-8.5
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69116 +/- ##
================================================
- Coverage 76.3235% 75.9141% -0.4095%
================================================
Files 2041 2068 +27
Lines 562593 592384 +29791
================================================
+ Hits 429391 449703 +20312
- Misses 132291 139375 +7084
- Partials 911 3306 +2395
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
e31b910 to
9bc0ae8
Compare
0xPoe
left a comment
0xPoe
left a comment
There was a problem hiding this comment.
Do we need to consider the same from:
// TableInfoByIDForInitStats returns the table info specified by the physicalID for initializing stats.
func (c *tableInfoGetterImpl) TableInfoByIDForInitStats(is infoschema.InfoSchema, physicalID int64) (table.Table, bool) {
// NOTE: For InfoSchema V2, partition lookups are efficient,
// We can directly search for partitioned tables without maintaining a separate cache, making TableByID sufficient for both
// regular tables and partitions.
isV2, _ := infoschema.IsV2(is)
if isV2 {
return c.TableInfoByID(is, physicalID)
}
// For InfoSchema V1, partition lookups require a full scan of all tables, which is
// expensive. We maintain a cached pid2tid map to optimize these lookups during stats
// initialization. Locking is needed because this method may be called in parallel.
// See FindTableByPartitionID implementation for more details on V1's search cost.
c.forInitStatsAndInfoSchemaV1Only.mu.Lock()
defer c.forInitStatsAndInfoSchemaV1Only.mu.Unlock()
if is.SchemaMetaVersion() != c.forInitStatsAndInfoSchemaV1Only.schemaVersion {
c.forInitStatsAndInfoSchemaV1Only.schemaVersion = is.SchemaMetaVersion()
c.forInitStatsAndInfoSchemaV1Only.pid2tid = buildPartitionID2TableID(is)
}
if id, ok := c.forInitStatsAndInfoSchemaV1Only.pid2tid[physicalID]; ok {
return is.TableByID(context.Background(), id)
}
return is.TableByID(context.Background(), physicalID)
}
This is called when we need to fill the table meta for the statsTbl. It needs to return a table info. So I think we don't need to change it. |
Co-authored-by: Dongpo Liu <poe.liu@pm.me>
|
/retest |
|
We discussed in IM. @0xPoe's concern is that the API we use may have perf regression if the cluster is under infoschemav1. This is a valid concern. I'm restructuring the pr. |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 0xPoe, terry1purcell, YangKeao The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
In response to a cherrypick label: new pull request created to branch |
5 participants
What problem does this PR solve?
Issue Number: close #69115
Problem Summary:
Stats initialization rebuilds the in-memory stats cache from
mysql.stats_meta. Dropped tables can leave stale stats metadata until stats GC removes it, so initialization should avoid loading stats entries whose physical table IDs no longer exist in the currentInfoSchema.What changed and how does it work?
This PR passes the current
InfoSchemainto the lite stats initialization path and validates each physical table ID while loading stats metadata.InfoSchemaare skipped.The tests cover both lite and non-lite stats initialization, including stale dropped-table metadata and valid partition physical IDs.
Check List
Tests
Side effects
Documentation
Unit test:
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests