Cherry pick 68492 v8.5.3 - #70080
Conversation
…in infoschema v2 Issue Number: close #00000 Problem Summary: Partition tables are currently classified as "special attributes" and their full PartitionInfo is stored in tableInfoResident, bypassing the Sieve cache. This violates the V2 design principle of lazy loading and causes significant memory bloat for large-scale partitioned table clusters. What changed and how does it work? 1. Removed PartitionAttribute from HasSpecialAttributes() filter - Partition tables no longer go into tableInfoResident (permanent memory) - PartitionInfo now loaded on-demand from Sieve cache (evictable) 2. Added GetPartitionedTableIDsV2() API - Extracts partitioned table IDs from pid2tid btree (version-aware) - Returns nil for V1 (fallback to ListTablesWithSpecialAttribute) - Zero additional memory overhead 3. Updated all callers to use V2 API with V1 fallback: - fetchShowStatsMeta / fetchShowStatsHealthy: Use GetPartitionedTableIDsV2 + on-demand TableInfoByID - rebuildAllPartitionValueMapAndSorted: Scan only LIST partitions using V2 API Expected Benefits: - Memory: ~100MB saved for 1000 partitioned tables × 100 partitions - Startup: ~95% reduction in LIST partition scan scope - Query Performance: No regression (partition pruning remains O(1)) See design doc: explorer/partition-table-optimization-design.md
- Remove issyncer files (not present in v8.5.3) - Revert GetAllNameToIDAndTheMustLoadedTableInfo to original 3-return signature - Replace GetNonPseudoPhysicalTableStats with GetPartitionStats/GetTableStats - Keep core changes: HasSpecialAttributes partition removal, GetPartitionedTableIDsV2
ti-chi-bot
Bot
added
do-not-merge/invalid-title
release-note-none
|
[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 |
📝 WalkthroughWalkthroughThe PR adds partition ID propagation through schema loading, partition-aware InfoSchema V2 DDL tracking and garbage collection, V2-based statistics and bootstrap discovery, and PD safe-point-aware schema GC. Related interfaces, tests, and Bazel test sharding are updated. ChangesPartition-aware InfoSchema changes
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Domain
participant MetaReader
participant InfoSchemaBuilder
participant InfoSchemaV2
Domain->>MetaReader: load table metadata and partition mappings
MetaReader-->>Domain: return partitionID2TableID
Domain->>InfoSchemaBuilder: initialize V2 with partition mappings
InfoSchemaBuilder->>InfoSchemaV2: populate pid2tid
InfoSchemaV2->>InfoSchemaV2: apply partition DDL deltas and tombstones
Suggested reviewers: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/infoschema/context/infoschema.go (1)
85-92: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd partition-aware coverage for
AllSpecialAttribute.
AllSpecialAttributeis the primary "all special tables" filter, and V2’s partition-table path only replacesPartitionAttribute. If the consumer atpkg/infoschema/infoschema_v2.go:2064-2076is meant to update bundles for all tables, add a partition-table fallback to the test/data path too rather than relying only on the V2 implementation note.🤖 Prompt for 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. In `@pkg/infoschema/context/infoschema.go` around lines 85 - 92, Add partition-aware handling to the AllSpecialAttribute filter by updating HasSpecialAttributes so partition tables are included through the test/data path, while preserving the existing special-attribute checks and V2’s PartitionAttribute exclusion behavior. Ensure consumers using AllSpecialAttribute cover all tables, including partitioned tables.
🧹 Nitpick comments (8)
pkg/session/bootstrap.go (1)
3602-3605: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the error and table identity in the warning.
The rebuild failure is logged without
error the table ID, making it undiagnosable.As per coding guidelines: "Keep error handling actionable and contextual; avoid silently swallowing errors".
♻️ Proposed change
- logutil.BgLogger().Warn("build list column partition value map and sorted failed") + logutil.BgLogger().Warn("build list column partition value map and sorted failed", + zap.Int64("tableID", tid), zap.Error(err))🤖 Prompt for 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. In `@pkg/session/bootstrap.go` around lines 3602 - 3605, Update the warning in the RebuildPartitionValueMapAndSorted error branch to include the returned err and the affected table’s identity, using the available table ID and structured context. Preserve the existing break behavior after logging the actionable failure details.Source: Coding guidelines
pkg/executor/show_stats.go (1)
118-131: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTake a single InfoSchema snapshot.
do.InfoSchema()is called at Line 118 and again at Line 130; the latest infoschema can advance in between, so the schema-name list and the partition map/table enumeration may come from different versions. Reuse oneis.♻️ Proposed change
- dbs := do.InfoSchema().AllSchemaNames() + is := do.InfoSchema() + dbs := is.AllSchemaNames() @@ partitionedTables := make(map[int64]*model.TableInfo) - is := do.InfoSchema() partitionedTableIDs := infoschema.GetPartitionedTableIDsV2(is, is.SchemaMetaVersion())🤖 Prompt for 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. In `@pkg/executor/show_stats.go` around lines 118 - 131, Reuse a single InfoSchema snapshot in the surrounding stats-building flow: assign do.InfoSchema() once to is before retrieving schema names, then derive dbs from is and use that same is for GetPartitionedTableIDsV2 and subsequent table enumeration. Remove the earlier direct do.InfoSchema() call while preserving existing behavior.pkg/infoschema/infoschema_v2.go (2)
117-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment contradicts the drop-partition branch.
Lines 121-122 state an empty non-nil map is returned for drop partition "so that callers skip re-inserting all remaining partitions", but Lines 138-145 return all remaining partition IDs precisely so they do get re-inserted as alive entries at the new version. Lines 134-137 describe the actual behavior correctly, so the earlier paragraph is leftover from a previous iteration and is actively misleading.
✏️ Remove the stale paragraph
// For truncate partition, these are the new replacement partition IDs. -// For drop partition, no new partitions are added, so an empty (non-nil) map is returned -// so that callers skip re-inserting all remaining partitions into pid2tid. +// For drop partition, the remaining partitions are returned so they get fresh alive +// entries at the new version; dropped ones are tomb-marked separately. // Returns nil if all partitions should be inserted (non-partition DDL, unable to diff, or no diff found).🤖 Prompt for 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. In `@pkg/infoschema/infoschema_v2.go` around lines 117 - 145, Update the doc comment for diffChangedPartitionIDs to remove the stale statement that drop partition returns an empty non-nil map and skips reinserting remaining partitions. Keep the documentation describing the actual behavior: drop partition returns the remaining partition IDs so they receive alive entries at the new version.
374-389: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove (or demote) the per-table
Infologs — this is a hot path.
addPartitionsruns once per partitioned table on every full load and once per partition DDL. Emitting an unconditionalInfoline each time floods the log on clusters with many partitioned tables, and the messages carry no actionable signal. These look like leftover debug instrumentation.🧹 Drop the logs
if pi := ti.GetPartitionInfo(); pi != nil { if changedPartitionIDs != nil { - cnt := 0 for _, def := range pi.Definitions { if _, ok := changedPartitionIDs[def.ID]; ok { btreeSet(&isd.pid2tid, partitionItem{def.ID, item.schemaVersion, tbl.Meta().ID, false}) - cnt++ } } - logutil.BgLogger().Info("infoschema v2 addPartitions delta", zap.Int("total_partitions", len(pi.Definitions)), zap.Int("inserted", cnt)) } else { - logutil.BgLogger().Info("infoschema v2 addPartitions full", zap.Int("total_partitions", len(pi.Definitions))) for _, def := range pi.Definitions { btreeSet(&isd.pid2tid, partitionItem{def.ID, item.schemaVersion, tbl.Meta().ID, false}) } } }The
pid2tid GC skippedlog at Line 664 has the same problem and is covered in the GC comment.🤖 Prompt for 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. In `@pkg/infoschema/infoschema_v2.go` around lines 374 - 389, Remove the unconditional Info logging from the addPartitions partition-update flow, including both the delta and full-load branches around pi.Definitions and changedPartitionIDs. Preserve the existing partition index updates and counters, and do not add replacement per-table logging.pkg/domain/db_test.go (1)
154-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting the new
pid2tidreturn rather than discarding it.
TestTetchAllSchemasWithTablesalready creates tables here; adding a partitioned one and asserting its partition IDs map to the table ID would give the aggregation path (including the concurrentpartitionIDMapmerge) its only direct unit coverage.💚 Sketch
tk.MustExec("create table t2(i int, s varchar(20), index index_t(i, s))") + tk.MustExec("create table tp(a int) partition by hash(a) partitions 4") tk.MustExec("create database test2") - dbs, _, err = domain.FetchAllSchemasWithTables(m) + dbs, pid2tid, err := domain.FetchAllSchemasWithTables(m) require.NoError(t, err) require.Equal(t, len(dbs), 5) + require.NotEmpty(t, pid2tid)(
mis a snapshot reader pinned atMaxUint, so confirm it observes the newly created table before asserting.)🤖 Prompt for 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. In `@pkg/domain/db_test.go` around lines 154 - 162, Update TestTetchAllSchemasWithTables to capture the pid2tid return from FetchAllSchemasWithTables instead of discarding it. Create a partitioned table, ensure the snapshot reader observes it, and assert each partition ID maps to the expected table ID to cover the partitionIDMap aggregation.pkg/meta/meta.go (1)
1206-1213: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment is inaccurate — extraction runs for all tables, not only non-must-loaded ones.
The block is outside the
isTableInfoMustLoadbranch, which is correct (must-load partitioned tables also needpid2tid), but the wording suggests otherwise.✏️ Wording fix
- // For non-must-loaded tables, still extract partition IDs from the raw JSON - // to populate pid2tid for FindTableByPartitionID etc. + // Extract partition IDs from the raw JSON for every table (must-loaded or not) + // to populate pid2tid for FindTableByPartitionID etc.🤖 Prompt for 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. In `@pkg/meta/meta.go` around lines 1206 - 1213, Update the comment above the ExtractPartitionIDs block to state that partition IDs are extracted for all tables, including must-loaded partitioned tables, to populate partitionID2TableID for FindTableByPartitionID and related lookups. Leave the extraction logic unchanged.pkg/infoschema/builder.go (1)
63-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc says
createSchemaTablesForDB, but the field is consumed inInitWithDBInfos(Lines 944-957).Also worth noting for future readers:
oldTableForPartitionDiffis implicit cross-method state betweenapplyTableUpdateV2andapplyCreateTable. It is reset at both ends today, but passing the old table explicitly throughapplyCreateTable's parameter list would remove the ordering coupling entirely.✏️ Doc fix
// partitionID2TableID is a temporary field used during full load (fast path) to // fill the pid2tid btree for partition tables that are not in the must-load list. - // It is set via WithPartitionID2TableID and consumed in createSchemaTablesForDB. + // It is set via WithPartitionID2TableID and consumed (then cleared) in InitWithDBInfos. partitionID2TableID map[int64]int64🤖 Prompt for 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. In `@pkg/infoschema/builder.go` around lines 63 - 70, Update the partitionID2TableID field comment to state that the map is consumed by InitWithDBInfos, replacing the inaccurate createSchemaTablesForDB reference. Leave the oldTableForPartitionDiff note and surrounding behavior unchanged.pkg/domain/domain.go (1)
480-484: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winTake the lock once per database instead of once per partition.
fetchSchemasWithTablesalready holds a fully-built per-DBpid2tidmap, so the merge at Lines 569-571 acquires/releases the mutex for every partition in the cluster. A bulkstoreAllcollapses that to one acquisition per DB.♻️ Bulk merge helper
func (pm *partitionIDMap) store(pid, tid int64) { pm.mu.Lock() defer pm.mu.Unlock() pm.m[pid] = tid } + +func (pm *partitionIDMap) storeAll(src map[int64]int64) { + if len(src) == 0 { + return + } + pm.mu.Lock() + defer pm.mu.Unlock() + for pid, tid := range src { + pm.m[pid] = tid + } +}And at the call site:
- // Merge per-DB partition mappings into the shared thread-safe map - for pid, tid := range pid2tid { - pm.store(pid, tid) - } + // Merge per-DB partition mappings into the shared thread-safe map. + pm.storeAll(pid2tid)🤖 Prompt for 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. In `@pkg/domain/domain.go` around lines 480 - 484, Update partitionIDMap by adding a bulk storeAll operation that locks once and merges a complete pid2tid map, then change fetchSchemasWithTables to use storeAll instead of calling store for each partition. Preserve the existing map contents and synchronization behavior while reducing locking to one acquisition per database.
🤖 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/executor/show_stats.go`:
- Around line 159-175: The duplicated partitionedTables construction in
fetchShowStatsMeta (pkg/executor/show_stats.go:159-175) and
fetchShowStatsHealthy (pkg/executor/show_stats.go:593-609) can dereference nil
partition info. Extract the map-building logic into a shared helper that
excludes tables where GetPartitionInfo() returns nil, then use that helper at
both call sites and remove the unguarded pi.Definitions dereferences.
In `@pkg/infoschema/BUILD.bazel`:
- Line 95: Fix the indentation of the embed attribute in the generated Bazel
target to match its sibling attributes, using four spaces. Leave shard_count and
other auto-derived generated values unchanged.
In `@pkg/infoschema/infoschema_v2.go`:
- Around line 199-217: Update the ActionReorganizePartition branch in
diffDroppedPartitionIDs to return an empty dropped-partition set when
diff.AffectedOpts is empty, matching intermediate drop/truncate-partition
handling. Preserve collecting OldTableID values when replacement options are
present, and prevent the branch from falling through to nil.
- Around line 2123-2136: Gate the call to
infoschemaV2.listPartitionTablesWithFilter from the surrounding table-listing
method so it runs only when filter is the PartitionAttribute filter; continue
returning the resident-table results unchanged for all other filters. Within
listPartitionTablesWithFilter, also replace the per-result linear scan used to
associate loaded table information with ret entries with a map keyed by table
name so each lookup is O(1).
- Around line 1760-1771: Update GetPartitionedTableIDsV2 by removing the
dangling design-document reference from its comment and using IsV2’s returned
boolean: bind ok and isv2, return nil when ok is false, then retrieve
partitioned table IDs from isv2.Data.
- Around line 658-679: Update gcOldPID2TIDVersion so lastGCPartitionCutVer is
advanced only after a successful, non-truncated collection and CompareAndSwap of
the new pid2tid tree. Ensure runs that return the 1024-item limit or lose the
CAS remain eligible for retry at the same schemaVersion, while preserving the
skip optimization only for fully completed successful runs.
In `@pkg/meta/BUILD.bazel`:
- Around line 42-43: Regenerate pkg/meta/BUILD.bazel using make bazel_prepare
rather than editing the generated file manually. Preserve the existing flaky and
shard_count values, but ensure flaky matches the four-space indentation of
sibling go_test attributes and the generated output passes Bazel formatting
checks.
In `@pkg/meta/meta.go`:
- Around line 1137-1152: Update ExtractPartitionIDs to return ([]int64, error),
perform a cheap byte-level check for partition information before decoding, and
unmarshal only the partition subtree rather than the full model.TableInfo.
Propagate JSON decoding errors instead of returning nil silently, then update
its callers and the assertions in meta_test.go to handle the error while
preserving nil IDs for tables without partition definitions.
In `@pkg/session/bootstrap.go`:
- Around line 3594-3600: Replace the assertion-only checks in the loop over
listPartitionTableIDs with production-safe handling: explicitly handle a false
result from is.TableByID before calling tbl.Meta(), and validate the
partitionExpr implementation before invoking PartitionExpr instead of relying on
tbl.(partitionExpr). Preserve the existing bootstrap error behavior by
propagating or returning an appropriate failure when either lookup or type
validation fails.
---
Outside diff comments:
In `@pkg/infoschema/context/infoschema.go`:
- Around line 85-92: Add partition-aware handling to the AllSpecialAttribute
filter by updating HasSpecialAttributes so partition tables are included through
the test/data path, while preserving the existing special-attribute checks and
V2’s PartitionAttribute exclusion behavior. Ensure consumers using
AllSpecialAttribute cover all tables, including partitioned tables.
---
Nitpick comments:
In `@pkg/domain/db_test.go`:
- Around line 154-162: Update TestTetchAllSchemasWithTables to capture the
pid2tid return from FetchAllSchemasWithTables instead of discarding it. Create a
partitioned table, ensure the snapshot reader observes it, and assert each
partition ID maps to the expected table ID to cover the partitionIDMap
aggregation.
In `@pkg/domain/domain.go`:
- Around line 480-484: Update partitionIDMap by adding a bulk storeAll operation
that locks once and merges a complete pid2tid map, then change
fetchSchemasWithTables to use storeAll instead of calling store for each
partition. Preserve the existing map contents and synchronization behavior while
reducing locking to one acquisition per database.
In `@pkg/executor/show_stats.go`:
- Around line 118-131: Reuse a single InfoSchema snapshot in the surrounding
stats-building flow: assign do.InfoSchema() once to is before retrieving schema
names, then derive dbs from is and use that same is for GetPartitionedTableIDsV2
and subsequent table enumeration. Remove the earlier direct do.InfoSchema() call
while preserving existing behavior.
In `@pkg/infoschema/builder.go`:
- Around line 63-70: Update the partitionID2TableID field comment to state that
the map is consumed by InitWithDBInfos, replacing the inaccurate
createSchemaTablesForDB reference. Leave the oldTableForPartitionDiff note and
surrounding behavior unchanged.
In `@pkg/infoschema/infoschema_v2.go`:
- Around line 117-145: Update the doc comment for diffChangedPartitionIDs to
remove the stale statement that drop partition returns an empty non-nil map and
skips reinserting remaining partitions. Keep the documentation describing the
actual behavior: drop partition returns the remaining partition IDs so they
receive alive entries at the new version.
- Around line 374-389: Remove the unconditional Info logging from the
addPartitions partition-update flow, including both the delta and full-load
branches around pi.Definitions and changedPartitionIDs. Preserve the existing
partition index updates and counters, and do not add replacement per-table
logging.
In `@pkg/meta/meta.go`:
- Around line 1206-1213: Update the comment above the ExtractPartitionIDs block
to state that partition IDs are extracted for all tables, including must-loaded
partitioned tables, to populate partitionID2TableID for FindTableByPartitionID
and related lookups. Leave the extraction logic unchanged.
In `@pkg/session/bootstrap.go`:
- Around line 3602-3605: Update the warning in the
RebuildPartitionValueMapAndSorted error branch to include the returned err and
the affected table’s identity, using the available table ID and structured
context. Preserve the existing break behavior after logging the actionable
failure details.
🪄 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 Plus
Run ID: d77b1146-c2bf-43aa-bb23-ce9633c2c4b6
📒 Files selected for processing (16)
pkg/domain/db_test.gopkg/domain/domain.gopkg/domain/test_helper.gopkg/executor/show_stats.gopkg/infoschema/BUILD.bazelpkg/infoschema/builder.gopkg/infoschema/cache.gopkg/infoschema/context/infoschema.gopkg/infoschema/infoschema_v2.gopkg/infoschema/infoschema_v2_test.gopkg/meta/BUILD.bazelpkg/meta/meta.gopkg/meta/meta_test.gopkg/meta/reader.gopkg/session/bootstrap.gotests/realtikvtest/sessiontest/infoschema_v2_test.go
| partitionedTable, ok := partitionedTables[tblID] | ||
| // Partitioned table: | ||
| if ok { | ||
| // For dynamic partitioned table, we need to display the global table as well. | ||
| if e.Ctx().GetSessionVars().IsDynamicPartitionPruneEnabled() { | ||
| stats := h.GetPartitionStats(partitionedTable, partitionedTable.ID) | ||
| if !stats.Pseudo { | ||
| e.appendTableForStatsMeta(db.O, partitionedTable.Name.O, "global", stats) | ||
| } | ||
| } | ||
| e.appendTableForStatsMeta(db.O, tbl.Name.O, partitionName, h.GetTableStats(tbl)) | ||
| if pi != nil { | ||
| for _, def := range pi.Definitions { | ||
| e.appendTableForStatsMeta(db.O, tbl.Name.O, def.Name.O, h.GetPartitionStats(tbl, def.ID)) | ||
| pi := partitionedTable.GetPartitionInfo() | ||
| for _, def := range pi.Definitions { | ||
| stats := h.GetPartitionStats(partitionedTable, def.ID) | ||
| if !stats.Pseudo { | ||
| e.appendTableForStatsMeta(db.O, partitionedTable.Name.O, def.Name.O, stats) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Unvalidated partition info in the duplicated V2 partition-map builder. Both stats fetchers build partitionedTables with the same copy-pasted block that inserts TableInfoByID results without checking GetPartitionInfo() != nil, then dereference pi unconditionally. Extract the map construction into one helper that filters out nil-partition tables, and both call sites become safe.
pkg/executor/show_stats.go#L159-L175: use the shared helper and drop the unguardedpi.Definitionsderef infetchShowStatsMeta.pkg/executor/show_stats.go#L593-L609: same change infetchShowStatsHealthy.
📍 Affects 1 file
pkg/executor/show_stats.go#L159-L175(this comment)pkg/executor/show_stats.go#L593-L609
🤖 Prompt for 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.
In `@pkg/executor/show_stats.go` around lines 159 - 175, The duplicated
partitionedTables construction in fetchShowStatsMeta
(pkg/executor/show_stats.go:159-175) and fetchShowStatsHealthy
(pkg/executor/show_stats.go:593-609) can dereference nil partition info. Extract
the map-building logic into a shared helper that excludes tables where
GetPartitionInfo() returns nil, then use that helper at both call sites and
remove the unguarded pi.Definitions dereferences.
| "sieve_test.go", | ||
| ], | ||
| embed = [":infoschema"], | ||
| embed = [":infoschema"], |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
embed is indented 8 spaces while every sibling attribute uses 4.
This is the signature of a hand-edit without re-running the generator, and it will trip the Bazel format check.
✏️ Fix
- embed = [":infoschema"],
+ embed = [":infoschema"],Leaving shard_count alone, since it is auto-derived. Based on learnings, flaky and shard_count on generated go_test targets should not be manually edited so the checked-in output doesn't diverge from make bazel_prepare.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| embed = [":infoschema"], | |
| embed = [":infoschema"], |
🤖 Prompt for 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.
In `@pkg/infoschema/BUILD.bazel` at line 95, Fix the indentation of the embed
attribute in the generated Bazel target to match its sibling attributes, using
four spaces. Leave shard_count and other auto-derived generated values
unchanged.
Source: Learnings
| case model.ActionReorganizePartition: | ||
| // Reorganize replaces some partitions with new ones. | ||
| // Old partitions should be tomb-marked. | ||
| // Use OldTableID for the same reason as truncate partition above. | ||
| if len(diff.AffectedOpts) > 0 { | ||
| dropped := make(map[int64]struct{}, len(diff.AffectedOpts)) | ||
| for _, opt := range diff.AffectedOpts { | ||
| dropped[opt.OldTableID] = struct{}{} | ||
| } | ||
| return dropped | ||
| } | ||
| } | ||
|
|
||
| // For add partition: no partitions are dropped, return empty set to skip tomb marking. | ||
| if diff.Type == model.ActionAddTablePartition { | ||
| return make(map[int64]struct{}) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Do reorganize-partition schema diffs ever carry empty AffectedOpts?
rg -nP -C6 'ActionReorganizePartition' --type=go -g '!**/*_test.go' pkg/ddl pkg/meta
rg -nP -C4 'AffectedOpts\s*=' --type=go pkg/ddlRepository: pingcap/tidb
Length of output: 36186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== schema_version.go relevant definitions/function =="
sed -n '190,220p' pkg/ddl/schema_version.go
echo
echo "== infoschema_v2.go relevant switch and applyDropTableV2 call site =="
sed -n '160,225p' pkg/infoschema/infoschema_v2.go
sed -n '1960,1990p' kg/infoschema/infoschema_v2.go || sed -n '1960,1990p' pkg/infoschema/infoschema_v2.go
echo
echo "== find addTableWithActionType / FindTableByPartitionID definitions =="
rg -n -C8 'func addTableWithActionType|func FindTableByPartitionID|func GetPartitionedTableIDs|func applyDropTableV2' pkg/infoschema/infoschema_v2.go pkg/infoschemaRepository: pingcap/tidb
Length of output: 8216
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== schema_version.go relevant definitions/function =="
sed -n '190,220p' pkg/ddl/schema_version.go
echo
echo "== infoschema_v2.go relevant switch and applyDropTableV2 call site =="
sed -n '160,225p' pkg/infoschema/infoschema_v2.go
sed -n '1955,1985p' pkg/infoschema/infoschema_v2.go
echo
echo "== find addTableWithActionType / FindTableByPartitionID definitions =="
rg -n -C8 'func addTableWithActionType|func FindTableByPartitionID|func GetPartitionedTableIDs|func applyDropTableV2' pkg/infoschema/infoschema_v2.go pkg/infoschemaRepository: pingcap/tidb
Length of output: 8165
Return empty dropped-partition set for intermediate ActionReorganizePartition states.
SetSchemaDiffForReorganizePartition leaves diff.AffectedOpts unset when args.NewPartitionIDs is empty, but the corresponding diffDroppedPartitionIDs call drops through to nil, causing applyDropTableV2 to tomb every existing partition at that version. Mirror the drop/truncate-partition intermediate-state handling and return make(map[int64]struct{}) when there are no replacement partition options.
🤖 Prompt for 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.
In `@pkg/infoschema/infoschema_v2.go` around lines 199 - 217, Update the
ActionReorganizePartition branch in diffDroppedPartitionIDs to return an empty
dropped-partition set when diff.AffectedOpts is empty, matching intermediate
drop/truncate-partition handling. Preserve collecting OldTableID values when
replacement options are present, and prevent the branch from falling through to
nil.
| // gcOldPID2TIDVersion performs GC of old partitionItem entries up to maxItems. | ||
| func (isd *Data) gcOldPID2TIDVersion(schemaVersion int64) int { | ||
| // Skip the expensive tree iteration if cutVer hasn't advanced since last GC. | ||
| // gcCollectPartitionItem can only delete entries with version < cutVer, | ||
| // so if cutVer is unchanged, the result would be identical to the last call. | ||
| if schemaVersion <= isd.lastGCPartitionCutVer.Load() { | ||
| logutil.BgLogger().Info("infoschema v2 pid2tid GC skipped (cutVer unchanged)", zap.Int64("schemaVersion", schemaVersion)) | ||
| return 0 | ||
| } | ||
| isd.lastGCPartitionCutVer.Store(schemaVersion) | ||
|
|
||
| old := isd.pid2tid.Load() | ||
| newTree := old.Clone() | ||
| dels := gcCollectPartitionItem(old, schemaVersion, 1024) | ||
| for _, item := range dels { | ||
| newTree.Delete(item) | ||
| } | ||
| if !isd.pid2tid.CompareAndSwap(old, newTree) { | ||
| logutil.BgLogger().Info("infoschema v2 GCOldVersion() pid2tid gc conflict") | ||
| } | ||
| return len(dels) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The cutVer skip can permanently strand entries when a GC run is truncated or its CAS fails.
Two paths leave pid2tid entries that are never reclaimed — and pid2tid is documented at Line 260 as needing full data in memory, so leaks here grow unbounded:
gcCollectPartitionItemis capped at 1024 items. When a run hits that cap there is still more to delete at the samecutVer, but the next call at an equal (or non-advancing) version returns early. The premise in the comment — "the result would be identical to the last call" — only holds when the previous run was not truncated.lastGCPartitionCutVer.Storehappens before the CAS. If the CAS loses (Line 675), the wholenewTreeis discarded, yet the version is already recorded, so that batch is never retried.
🐛 Only record the cut version on a complete, successful run
if schemaVersion <= isd.lastGCPartitionCutVer.Load() {
- logutil.BgLogger().Info("infoschema v2 pid2tid GC skipped (cutVer unchanged)", zap.Int64("schemaVersion", schemaVersion))
return 0
}
- isd.lastGCPartitionCutVer.Store(schemaVersion)
old := isd.pid2tid.Load()
newTree := old.Clone()
- dels := gcCollectPartitionItem(old, schemaVersion, 1024)
+ const maxItems = 1024
+ dels := gcCollectPartitionItem(old, schemaVersion, maxItems)
for _, item := range dels {
newTree.Delete(item)
}
if !isd.pid2tid.CompareAndSwap(old, newTree) {
logutil.BgLogger().Info("infoschema v2 GCOldVersion() pid2tid gc conflict")
+ return 0
}
+ // Only mark this cut version as fully processed when the run was not truncated,
+ // otherwise the remaining entries at this version would never be revisited.
+ if len(dels) < maxItems {
+ isd.lastGCPartitionCutVer.Store(schemaVersion)
+ }
return len(dels)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // gcOldPID2TIDVersion performs GC of old partitionItem entries up to maxItems. | |
| func (isd *Data) gcOldPID2TIDVersion(schemaVersion int64) int { | |
| // Skip the expensive tree iteration if cutVer hasn't advanced since last GC. | |
| // gcCollectPartitionItem can only delete entries with version < cutVer, | |
| // so if cutVer is unchanged, the result would be identical to the last call. | |
| if schemaVersion <= isd.lastGCPartitionCutVer.Load() { | |
| logutil.BgLogger().Info("infoschema v2 pid2tid GC skipped (cutVer unchanged)", zap.Int64("schemaVersion", schemaVersion)) | |
| return 0 | |
| } | |
| isd.lastGCPartitionCutVer.Store(schemaVersion) | |
| old := isd.pid2tid.Load() | |
| newTree := old.Clone() | |
| dels := gcCollectPartitionItem(old, schemaVersion, 1024) | |
| for _, item := range dels { | |
| newTree.Delete(item) | |
| } | |
| if !isd.pid2tid.CompareAndSwap(old, newTree) { | |
| logutil.BgLogger().Info("infoschema v2 GCOldVersion() pid2tid gc conflict") | |
| } | |
| return len(dels) | |
| } | |
| // gcOldPID2TIDVersion performs GC of old partitionItem entries up to maxItems. | |
| func (isd *Data) gcOldPID2TIDVersion(schemaVersion int64) int { | |
| // Skip the expensive tree iteration if cutVer hasn't advanced since last GC. | |
| // gcCollectPartitionItem can only delete entries with version < cutVer, | |
| // so if cutVer is unchanged, the result would be identical to the last call. | |
| if schemaVersion <= isd.lastGCPartitionCutVer.Load() { | |
| return 0 | |
| } | |
| old := isd.pid2tid.Load() | |
| newTree := old.Clone() | |
| const maxItems = 1024 | |
| dels := gcCollectPartitionItem(old, schemaVersion, maxItems) | |
| for _, item := range dels { | |
| newTree.Delete(item) | |
| } | |
| if !isd.pid2tid.CompareAndSwap(old, newTree) { | |
| logutil.BgLogger().Info("infoschema v2 GCOldVersion() pid2tid gc conflict") | |
| return 0 | |
| } | |
| // Only mark this cut version as fully processed when the run was not truncated, | |
| // otherwise the remaining entries at this version would never be revisited. | |
| if len(dels) < maxItems { | |
| isd.lastGCPartitionCutVer.Store(schemaVersion) | |
| } | |
| return len(dels) | |
| } |
🤖 Prompt for 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.
In `@pkg/infoschema/infoschema_v2.go` around lines 658 - 679, Update
gcOldPID2TIDVersion so lastGCPartitionCutVer is advanced only after a
successful, non-truncated collection and CompareAndSwap of the new pid2tid tree.
Ensure runs that return the 1024-item limit or lose the CAS remain eligible for
retry at the same schemaVersion, while preserving the skip optimization only for
fully completed successful runs.
| // GetPartitionedTableIDsV2 returns all partitioned table IDs for the given schema version. | ||
| // This is used to replace ListTablesWithSpecialAttribute(PartitionAttribute) in V2, | ||
| // allowing partition tables to be loaded on-demand from the Sieve cache. | ||
| // Returns nil if not V2 or no partitioned tables found. | ||
| // See design doc: explorer/partition-table-optimization-design.md | ||
| func GetPartitionedTableIDsV2(is InfoSchema, schemaVersion int64) []int64 { | ||
| _, isv2 := IsV2(is) | ||
| if isv2 == nil { | ||
| return nil | ||
| } | ||
| return isv2.Data.GetPartitionedTableIDs(schemaVersion) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Is the referenced design doc tracked in the repo?
fd -H -t f 'partition-table-optimization-design.md'
fd -H -t d '^explorer$'Repository: pingcap/tidb
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked docs/files containing 'partition-table-optimization-design' or 'explorer':"
git ls-files | grep -E '(^|/)partition-table-optimization-design\.md$|(^|/)explorer(/|$)' || true
echo
echo "All tracked files matching 'partition-table-optimization-design':"
git ls-files | grep 'partition-table-optimization-design' || true
echo
echo "Occurrences of referenced doc path in Go files:"
rg -n "explorer/partition-table-optimization-design\.md|partition-table-optimization-design\.md" --glob '*.go' || true
echo
echo "Relevant code around GetPartitionedTableIDsV2 and IsV2:"
fd -H -t f 'infoschema_v2\.go$' . | while read -r f; do
echo "--- ${f#"$PWD"/} ---"
wc -l "$f"
sed -n '1,240p' "$f" | cat -n
doneRepository: pingcap/tidb
Length of output: 349
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked docs/files containing 'partition-table-optimization-design' or 'explorer':"
git ls-files | grep -E '(^|/)partition-table-optimization-design\.md$|(^|/)explorer(/|$)' || true
echo
echo "All tracked files matching 'partition-table-optimization-design':"
git ls-files | grep 'partition-table-optimization-design' || true
echo
echo "Occurrences of referenced doc path in Go files:"
rg -n "explorer/partition-table-optimization-design\.md|partition-table-optimization-design\.md" --glob '*.go' || true
echo
echo "Relevant code around GetPartitionedTableIDsV2 and IsV2:"
fd -H -t f 'infoschema_v2\.go$' . | while read -r f; do
echo "--- ${f#"$PWD"/} ---"
wc -l "$f"
sed -n '1,240p' "$f" | cat -n
doneRepository: pingcap/tidb
Length of output: 349
Drop the dangling design-doc reference and use IsV2’s boolean flag.
explorer/partition-table-optimization-design.md is not a tracked file in the repository, so this exported helper’s doc comment should either point to an actual commit-local path or be removed. Also prefer ok, isv2 := IsV2(is); if !ok over ignoring the boolean and testing isv2 == nil.
♻️ Use the returned flag
func GetPartitionedTableIDsV2(is InfoSchema, schemaVersion int64) []int64 {
- _, isv2 := IsV2(is)
- if isv2 == nil {
+ ok, isv2 := IsV2(is)
+ if !ok {
return nil
}
return isv2.Data.GetPartitionedTableIDs(schemaVersion)
}🤖 Prompt for 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.
In `@pkg/infoschema/infoschema_v2.go` around lines 1760 - 1771, Update
GetPartitionedTableIDsV2 by removing the dangling design-document reference from
its comment and using IsV2’s returned boolean: bind ok and isv2, return nil when
ok is false, then retrieve partitioned table IDs from isv2.Data.
|
|
||
| // Partition tables are not stored in tableInfoResident (to save memory). | ||
| // Check pid2tid for partition tables and look up their table info on demand. | ||
| is.listPartitionTablesWithFilter(filter, &ret, existingTableIDs) | ||
|
|
||
| return ret | ||
| } | ||
|
|
||
| // listPartitionTablesWithFilter looks up partition tables from pid2tid, loads their | ||
| // table info, applies the filter, and appends matching results to ret. | ||
| // Partition tables are excluded from tableInfoResident to save memory, so we need | ||
| // this separate path to handle the PartitionAttribute filter. | ||
| // existingTableIDs tracks tables already found via tableInfoResident to avoid duplicates. | ||
| func (is *infoschemaV2) listPartitionTablesWithFilter(filter infoschemacontext.SpecialAttributeFilter, ret *[]infoschemacontext.TableInfoResult, existingTableIDs map[int64]bool) { |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Gate the partition pass on the filter — it currently force-loads every partitioned table for every attribute query.
listPartitionTablesWithFilter is called unconditionally, so a ListTablesWithSpecialAttribute(TTLAttribute) (polled periodically by the TTL job manager) now walks the whole pid2tid tree and issues TableInfoByID for every distinct partitioned table before discarding almost all of them via filter.
That is both expensive and self-defeating: each lookup pulls a full TableInfo into the Sieve cache, which is the memory this PR set out to save.
It is also unnecessary. Only PartitionAttribute was removed from HasSpecialAttributes; a partitioned table that has TTL / TiFlash / placement / locks / FKs still satisfies HasSpecialAttributes and is therefore still in tableInfoResident, so the first pass already covers every other filter.
🐛 Only run the partition pass when the filter needs it
- // Partition tables are not stored in tableInfoResident (to save memory).
- // Check pid2tid for partition tables and look up their table info on demand.
- is.listPartitionTablesWithFilter(filter, &ret, existingTableIDs)
+ // Partition tables are no longer stored in tableInfoResident (to save memory), so
+ // partition-aware filters need a separate pass over pid2tid. Every other special
+ // attribute is still resident, so skip the (expensive) on-demand loads for them.
+ if needsPartitionScan(filter) {
+ is.listPartitionTablesWithFilter(filter, &ret, existingTableIDs)
+ }needsPartitionScan can compare against the known partition-matching filters, or SpecialAttributeFilter can gain an explicit flag/identity so callers don't rely on function-pointer comparison.
Secondary nit for whenever the helper does run: the for i := range *ret DB lookup at Lines 2179-2185 is linear per table; a map[string]int index would keep it O(1).
🤖 Prompt for 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.
In `@pkg/infoschema/infoschema_v2.go` around lines 2123 - 2136, Gate the call to
infoschemaV2.listPartitionTablesWithFilter from the surrounding table-listing
method so it runs only when filter is the PartitionAttribute filter; continue
returning the resident-table results unchanged for all other filters. Within
listPartitionTablesWithFilter, also replace the per-result linear scan used to
associate loaded table information with ret entries with a map keyed by table
name so each lookup is O(1).
| flaky = True, | ||
| shard_count = 25, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Indentation of flaky diverges from generated output.
Line 42 is indented with 8 spaces while the sibling attributes use 4, which suggests a hand edit; buildifier/make bazel_prepare will rewrite it and CI's Bazel-file check may fail. Regenerate rather than editing by hand.
Based on learnings that flaky/shard_count on generated go_test targets are auto-derived during make bazel_prepare, the values themselves are fine — only the formatting needs regenerating.
🤖 Prompt for 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.
In `@pkg/meta/BUILD.bazel` around lines 42 - 43, Regenerate pkg/meta/BUILD.bazel
using make bazel_prepare rather than editing the generated file manually.
Preserve the existing flaky and shard_count values, but ensure flaky matches the
four-space indentation of sibling go_test attributes and the generated output
passes Bazel formatting checks.
Source: Learnings
| // ExtractPartitionIDs extracts partition IDs from a serialized TableInfo JSON. | ||
| // Returns nil if the table has no partition info or empty definitions. | ||
| func ExtractPartitionIDs(rawJSON []byte) []int64 { | ||
| ti := &model.TableInfo{} | ||
| if err := json.Unmarshal(rawJSON, ti); err != nil { | ||
| return nil | ||
| } | ||
| if ti.Partition == nil || len(ti.Partition.Definitions) == 0 { | ||
| return nil | ||
| } | ||
| ids := make([]int64, len(ti.Partition.Definitions)) | ||
| for i, def := range ti.Partition.Definitions { | ||
| ids[i] = def.ID | ||
| } | ||
| return ids | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Full TableInfo unmarshal per table defeats the must-load fast path.
ExtractPartitionIDs decodes the entire TableInfo (columns, indices, etc.) for every table in the DB, which is precisely the cost isTableInfoMustLoad exists to avoid when tidb_schema_cache_size > 0. With hundreds of thousands of tables this makes full load substantially more expensive.
Two things worth addressing:
- Gate on a cheap byte scan before unmarshalling, and decode only the partition subtree.
- Returning
nilon unmarshal failure silently drops the pid→tid mapping for that table, which later surfaces as a missingFindTableByPartitionIDresult rather than an error. Prefer returning the error to the caller (which already handles unmarshal errors on the must-load path).
♻️ Sketch of a cheaper, non-swallowing variant
-// ExtractPartitionIDs extracts partition IDs from a serialized TableInfo JSON.
-// Returns nil if the table has no partition info or empty definitions.
-func ExtractPartitionIDs(rawJSON []byte) []int64 {
- ti := &model.TableInfo{}
- if err := json.Unmarshal(rawJSON, ti); err != nil {
- return nil
- }
- if ti.Partition == nil || len(ti.Partition.Definitions) == 0 {
- return nil
- }
+// ExtractPartitionIDs extracts partition IDs from a serialized TableInfo JSON.
+// Returns nil if the table has no partition info or empty definitions.
+// Only the "partition" subtree is decoded to keep the must-load fast path cheap.
+func ExtractPartitionIDs(rawJSON []byte) ([]int64, error) {
+ // Cheap pre-check: skip the decode entirely for non-partitioned tables.
+ if !bytes.Contains(rawJSON, hack.Slice(`"partition":{`)) {
+ return nil, nil
+ }
+ var ti struct {
+ Partition *model.PartitionInfo `json:"partition"`
+ }
+ if err := json.Unmarshal(rawJSON, &ti); err != nil {
+ return nil, errors.Trace(err)
+ }
+ if ti.Partition == nil || len(ti.Partition.Definitions) == 0 {
+ return nil, nil
+ }
ids := make([]int64, len(ti.Partition.Definitions))
for i, def := range ti.Partition.Definitions {
ids[i] = def.ID
}
- return ids
+ return ids, nil
}Note pkg/meta/meta_test.go:661-700 asserts the current single-return signature, so it would need updating alongside.
🤖 Prompt for 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.
In `@pkg/meta/meta.go` around lines 1137 - 1152, Update ExtractPartitionIDs to
return ([]int64, error), perform a cheap byte-level check for partition
information before decoding, and unmarshal only the partition subtree rather
than the full model.TableInfo. Propagate JSON decoding errors instead of
returning nil silently, then update its callers and the assertions in
meta_test.go to handle the error while preserving nil IDs for tables without
partition definitions.
| for _, tid := range listPartitionTableIDs { | ||
| tbl, ok := is.TableByID(ctx, tid) | ||
| intest.Assert(ok, "table not found in infoschema") | ||
| tblInfo := tbl.Meta() | ||
| pi := tblInfo.GetPartitionInfo() | ||
| intest.AssertNotNil(pi) | ||
| pe := tbl.(partitionExpr).PartitionExpr() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
intest.Assert doesn't protect the production path.
intest.Assert is a no-op unless assertions are enabled, so a false from TableByID (e.g. on-demand load failure in V2) leaves tbl == nil and Line 3597 panics during bootstrap. Same for the unchecked tbl.(partitionExpr) assertion.
🐛 Proposed fix
for _, tid := range listPartitionTableIDs {
tbl, ok := is.TableByID(ctx, tid)
- intest.Assert(ok, "table not found in infoschema")
+ intest.Assert(ok, "table not found in infoschema")
+ if !ok {
+ logutil.BgLogger().Warn("table not found in infoschema", zap.Int64("tableID", tid))
+ continue
+ }
tblInfo := tbl.Meta()
pi := tblInfo.GetPartitionInfo()
intest.AssertNotNil(pi)
- pe := tbl.(partitionExpr).PartitionExpr()
+ pex, ok := tbl.(partitionExpr)
+ if pi == nil || !ok {
+ continue
+ }
+ pe := pex.PartitionExpr()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, tid := range listPartitionTableIDs { | |
| tbl, ok := is.TableByID(ctx, tid) | |
| intest.Assert(ok, "table not found in infoschema") | |
| tblInfo := tbl.Meta() | |
| pi := tblInfo.GetPartitionInfo() | |
| intest.AssertNotNil(pi) | |
| pe := tbl.(partitionExpr).PartitionExpr() | |
| for _, tid := range listPartitionTableIDs { | |
| tbl, ok := is.TableByID(ctx, tid) | |
| intest.Assert(ok, "table not found in infoschema") | |
| if !ok { | |
| logutil.BgLogger().Warn("table not found in infoschema", zap.Int64("tableID", tid)) | |
| continue | |
| } | |
| tblInfo := tbl.Meta() | |
| pi := tblInfo.GetPartitionInfo() | |
| intest.AssertNotNil(pi) | |
| pex, ok := tbl.(partitionExpr) | |
| if pi == nil || !ok { | |
| continue | |
| } | |
| pe := pex.PartitionExpr() |
🤖 Prompt for 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.
In `@pkg/session/bootstrap.go` around lines 3594 - 3600, Replace the
assertion-only checks in the loop over listPartitionTableIDs with
production-safe handling: explicitly handle a false result from is.TableByID
before calling tbl.Meta(), and validate the partitionExpr implementation before
invoking PartitionExpr instead of relying on tbl.(partitionExpr). Preserve the
existing bootstrap error behavior by propagating or returning an appropriate
failure when either lookup or type validation fails.
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the The title description (the part after 📖 For more info, you can check the "Contribute Code" section in the development guide. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5-20250917-v8.5.3 #70080 +/- ##
================================================================
Coverage ? 57.3007%
================================================================
Files ? 1780
Lines ? 631793
Branches ? 0
================================================================
Hits ? 362022
Misses ? 245431
Partials ? 24340
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/test pull-br-integration-test |
|
/retest |
1 similar comment
|
/retest |
|
@tiancaiamao: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/test pull-br-integration-test |
|
@tiancaiamao: The specified target(s) for The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
2 participants
What problem does this PR solve?
Issue Number: close #xxx
Problem Summary:
What changed and how does it work?
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
New Features
Bug Fixes
Tests