Hotfix/hf 1056/v6.5.11#69943
Conversation
…#684) (pingcap#685) Signed-off-by: HunDunDM <hundundm@gmail.com>
Reviewed-on: https://git.pingcap.net/pingkai/tidb/pulls/715 Reviewed-by: crazycs520 <chenshuang@pingcap.cn>
close pingcap#54648 (cherry picked from commit 8471e8a)
close pingcap#54777 (cherry picked from commit 60c7e61)
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 |
ti-chi-bot
Bot
added
contribution
|
Hi @lichunzhu. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
tangenta seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
📝 WalkthroughWalkthroughThis PR adds stats-owner configuration and lifecycle coordination, table-ID-scoped backfill metrics, PD force-merge range generation and submission, DDL ownership and event updates, planner and executor correctness fixes, metadata persistence, HTTP reset support, and regression tests. ChangesRuntime ownership and configuration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
|
[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. |
|
/ok-to-test |
|
@lichunzhu: Cannot trigger testing until a trusted user reviews the PR and leaves an 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. |
|
/ok-to-test |
ti-chi-bot
Bot
added
ok-to-test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ddl/reorg.go (1)
1-1: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftClear partition backfill metrics by the same ID scope they were registered with
ddl/reorg.go#L863-L865:CleanupDDLReorgHandlesonly clearsjob.TableID, but partitioned ADD INDEX / MODIFY COLUMN / merge-temp / non-drop cleanup-index backfill metrics are registered under the physical partition ID. Those entries survive job completion and leave stale Prometheus series behind. Clear every physical partition ID touched by the job, or make the registration use the same ID scope as cleanup.🤖 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 `@ddl/reorg.go` at line 1, Update CleanupDDLReorgHandles to clear backfill metrics for every physical partition ID touched by the job, not only job.TableID. Align cleanup with the partition-ID registration used by partitioned ADD INDEX, MODIFY COLUMN, merge-temp, and non-drop cleanup-index operations, while preserving existing cleanup for non-partitioned jobs.owner/mock.go (1)
64-66: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
mockManager.RetireOwnerdoesn't invoke the new retire hook, breaking theManagerinterface contract.
owner/manager.go'sownerManager.RetireOwner()was updated to callretireOwnerHookbefore clearing ownership state, butmockManager.RetireOwner()(unchanged, lines 64-66) never callsm.retireHook. Sincedomain.newOwnerManagerfalls back toowner.NewMockManagerwheneverdo.etcdClient == nil(standalone deployments, most unit tests), the hookddl.Start()registers viaSetRetireOwnerHookto resetd.runningJobson retirement will silently never fire in these environments, leavingrunningJobsstate stale after a retirement.🐛 Proposed fix
func (m *mockManager) RetireOwner() { + if m.retireHook != nil { + m.retireHook() + } atomic.StoreInt32(&m.owner, 0) }Also applies to: 100-109
🤖 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 `@owner/mock.go` around lines 64 - 66, Update mockManager.RetireOwner to invoke the registered m.retireHook before clearing the ownership state, matching ownerManager.RetireOwner and preserving the Manager contract. Ensure the hook is called safely when no hook has been registered, then perform the existing atomic.StoreInt32 operation.
🧹 Nitpick comments (4)
domain/infosync/pkdb_force_merge_test.go (1)
226-246: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReset path bypasses
atomic.Value's API.
globalInfoSyncer = atomic.Value{}directly overwrites the package-levelatomic.Valuevariable instead of using.Store(...). This skips the atomicity guarantees the type exists to provide and is inconsistent with the.Load()/setGlobalInfoSyncer(...)(presumably.Store(...)) calls used everywhere else in this helper.♻️ Proposed fix
t.Cleanup(func() { if oldInfoSyncer != nil { setGlobalInfoSyncer(oldInfoSyncer) } else { - globalInfoSyncer = atomic.Value{} + globalInfoSyncer.Store((*InfoSyncer)(nil)) } getPDAddrsForForceMergeFunc = oldGetPDAddrsForForceMergeFunc })🤖 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 `@domain/infosync/pkdb_force_merge_test.go` around lines 226 - 246, Update the cleanup path in the helper that restores globalInfoSyncer to reset it through the atomic.Value API, using the existing setGlobalInfoSyncer or an equivalent Store-based operation instead of assigning a new atomic.Value. Preserve restoration of oldInfoSyncer when present and continue restoring getPDAddrsForForceMergeFunc.domain/infosync/info.go (1)
425-433: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
doRequest's body factory doesn't actually recreate the body per attempt.
bodyFactory = func() io.Reader { return body }returns the same, potentially-exhausted reader on every retry acrossaddrs, unlike the fresh-reader fix applied indoRequestWithBodyBytes. Any existing caller ofdoRequestthat passes a non-rewindableio.Reader(or one that gets consumed on first attempt) will still silently send an empty/partial body on retry to subsequent PD addresses — the exact class of bug this PR fixes for the new force-merge path.Consider migrating remaining
doRequestcallers to buffer their bodies as[]byteand usedoRequestWithBodyBytes, or accept only pre-buffered bytes here too.#!/bin/bash # Find existing callers of doRequest with a non-nil body to assess whether they're affected. rg -n -B3 -A3 '\bdoRequest\(' --type=go domain/infosync🤖 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 `@domain/infosync/info.go` around lines 425 - 433, Update doRequest so retries receive a fresh reader instead of returning the same potentially consumed body; preferably buffer non-nil request bodies and delegate to doRequestWithBodyBytes, or otherwise constrain this path to reusable buffered data. Review existing doRequest callers with request bodies and migrate them as needed, preserving empty-body behavior.planner/core/planbuilder.go (1)
809-837: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFilter on
_tidb_rowidbefore doing infoschema lookups.
fieldNameToColumnNamerunsTableIsView/TableByNamelookups unconditionally, but the caller only cares about_tidb_rowidcolumns (filtered later inappendPartitionRowIDWarning). Since this is invoked from three sites intoColumn's hot path for every resolved column in every query, doing the name check first avoids the extra infoschema lookups for the common case.⚡ Proposed fix
func (b *PlanBuilder) fieldNameToColumnName(fieldName *types.FieldName) *ast.ColumnName { if b == nil || fieldName == nil { return nil } colNameCI := fieldName.OrigColName if colNameCI.L == "" { colNameCI = fieldName.ColName } + if colNameCI.L != model.ExtraHandleName.L { + return nil + } tblNameCI := fieldName.OrigTblName if tblNameCI.L == "" { tblNameCI = fieldName.TblName }🤖 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 `@planner/core/planbuilder.go` around lines 809 - 837, Update fieldNameToColumnName to return immediately unless fieldName represents the _tidb_rowid column, before calling TableIsView or TableByName; preserve the existing column-name construction and lookup behavior for _tidb_rowid, since appendPartitionRowIDWarning is the caller that consumes this result.infoschema/builder.go (1)
808-824: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winShort-circuit the cheap
DDLHasBackfillMetrics()check before scanning all columns/indices.
allColumnPublic/allIndexPublicare computed viaslices.ContainsFuncover every column and index unconditionally, before themetrics.DDLHasBackfillMetrics()check even runs (it's just the last operand of the&&, evaluated after both scans complete). SinceapplyCreateTableruns on essentially every table-affecting schema diff, reordering avoids the scan entirely in the common case where no backfill metrics are active.♻️ Proposed reorder
- allColumnPublic := !slices.ContainsFunc(tblInfo.Columns, - func(col *model.ColumnInfo) bool { - return col.State != model.StatePublic - }) - allIndexPublic := !slices.ContainsFunc(tblInfo.Indices, - func(idx *model.IndexInfo) bool { - return idx.State != model.StatePublic - }) - if allColumnPublic && allIndexPublic && metrics.DDLHasBackfillMetrics() { + if metrics.DDLHasBackfillMetrics() { + allColumnPublic := !slices.ContainsFunc(tblInfo.Columns, + func(col *model.ColumnInfo) bool { + return col.State != model.StatePublic + }) + allIndexPublic := !slices.ContainsFunc(tblInfo.Indices, + func(idx *model.IndexInfo) bool { + return idx.State != model.StatePublic + }) + if allColumnPublic && allIndexPublic { metrics.DDLClearBackfillMetrics(tblInfo.ID) if tblInfo.Partition != nil { for _, def := range tblInfo.Partition.Definitions { metrics.DDLClearBackfillMetrics(def.ID) } } + } }🤖 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 `@infoschema/builder.go` around lines 808 - 824, In the backfill-metric cleanup block, evaluate metrics.DDLHasBackfillMetrics() before computing allColumnPublic and allIndexPublic so the column/index scans are skipped when no metrics are active. Preserve the existing cleanup behavior, including partition definition handling, when backfill metrics are present and both visibility checks pass.
🤖 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 `@ddl/column.go`:
- Line 1205: Update the ActionModifyColumn rate-metric registration around
metricCounter to use the logical table ID expected by the shared cleanup flow
instead of backfillMetricsTableID, which resolves to a physical partition ID.
Align this registration with the table-ID scope validated by the backfill
metrics audit tests and the consolidated reorg handling.
In `@ddl/index_merge_tmp.go`:
- Around line 143-147: Update the metric registration in the worker
initialization around backfillWorker and conflictCounter so both counters use
the logical table ID expected by the reorg metrics contract rather than
PhysicalTableID. Apply the same table-ID selection consistently to
getBackfillTotalByTableID for the normal and conflict metrics, matching the
consolidated handling in reorg.go.
In `@ddl/index.go`:
- Line 1319: Update the ActionAddIndex metricCounter initialization around
getBackfillTotalByTableID and backfillMetricsTableID to register the backfill
metric under the same table-ID scope used by ddl/reorg.go, rather than
PhysicalTableID. Keep the existing metric label, schema, table name, and index
name arguments unchanged, and align the identifier with the other ActionAddIndex
registration paths.
- Line 1854: Update the metricCounter registration using
getBackfillTotalByTableID so non-partition-drop actions use the expected table
identifier instead of PhysicalTableID, matching the key used by ddl/reorg.go.
Preserve the existing identifier behavior for partition-drop actions and keep
the current metric label and schema/name arguments unchanged.
In `@ddl/schema.go`:
- Line 23: Update the drop-schema notification flow that emits ActionDropTable
events so notifications are batched or otherwise delivered reliably instead of
sending one best-effort event per table through asyncNotifyEvent. Ensure the
implementation avoids exhausting the bounded ddlEventCh and does not lose events
after retry limits, while preserving cleanup for every dropped table.
In `@distsql/request_builder.go`:
- Around line 774-776: Update the memTracker.Consume calculation in the krs
initialization path to multiply the KeyRange struct size by both len(ranges) and
len(tids), matching the per-table-ID slice capacity allocation. Preserve the
existing nil-check and memory accounting behavior.
In `@domain/domain.go`:
- Around line 1900-1902: Update the stats initialization flow around the
statsLease guard so do.statsOwner is canceled before returning when
do.statsLease <= 0. Preserve the existing behavior of skipping updateStatsWorker
while ensuring the unconditionally created owner manager and its campaign
resources are cleaned up.
- Around line 1918-1921: Update Domain.disableStatsOwner to safely handle cases
where statsOwner or its CampaignCancel callback was not initialized because
CampaignOwner was skipped. Initialize the callback to a no-op or guard the call
before invoking CampaignCancel, while preserving the existing successful disable
behavior.
- Around line 1911-1916: Update Domain.enableStatsOwner to prevent overlapping
CampaignOwner calls while an election is already in progress, tracking the
active campaign attempt separately from statsOwner.IsOwner(). Preserve the
existing owner fast path, ensure repeated calls reuse or return the in-flight
campaign result, and prevent later attempts from overwriting the shared
campaignCancel used for shutdown.
In `@executor/distsql.go`:
- Around line 458-465: Remove the duplicate memTracker initialization/reset
block from the later open() path, while retaining the single reset or creation
before buildTableKeyRanges(). Ensure the existing AttachTo call uses that same
tracker and preserves memory consumed while generating key ranges.
In `@metrics/ddl.go`:
- Around line 224-244: Align the ID passed to GetBackfillTotalByTableID and
GetBackfillProgressByTableID with the ID used by DDLClearBackfillMetrics for the
same DDL action. Update the relevant registration and cleanup paths, including
CleanupDDLReorgHandles, so both consistently use the same table-ID scope and
registered labels can be removed.
In `@planner/core/rule_column_pruning.go`:
- Around line 97-113: Update the error message in noZeroColumnLayOut to say
“zero column output” instead of “zero row output,” while preserving the existing
schema passthrough check and LogicalTableDual exception.
---
Outside diff comments:
In `@ddl/reorg.go`:
- Line 1: Update CleanupDDLReorgHandles to clear backfill metrics for every
physical partition ID touched by the job, not only job.TableID. Align cleanup
with the partition-ID registration used by partitioned ADD INDEX, MODIFY COLUMN,
merge-temp, and non-drop cleanup-index operations, while preserving existing
cleanup for non-partitioned jobs.
In `@owner/mock.go`:
- Around line 64-66: Update mockManager.RetireOwner to invoke the registered
m.retireHook before clearing the ownership state, matching
ownerManager.RetireOwner and preserving the Manager contract. Ensure the hook is
called safely when no hook has been registered, then perform the existing
atomic.StoreInt32 operation.
---
Nitpick comments:
In `@domain/infosync/info.go`:
- Around line 425-433: Update doRequest so retries receive a fresh reader
instead of returning the same potentially consumed body; preferably buffer
non-nil request bodies and delegate to doRequestWithBodyBytes, or otherwise
constrain this path to reusable buffered data. Review existing doRequest callers
with request bodies and migrate them as needed, preserving empty-body behavior.
In `@domain/infosync/pkdb_force_merge_test.go`:
- Around line 226-246: Update the cleanup path in the helper that restores
globalInfoSyncer to reset it through the atomic.Value API, using the existing
setGlobalInfoSyncer or an equivalent Store-based operation instead of assigning
a new atomic.Value. Preserve restoration of oldInfoSyncer when present and
continue restoring getPDAddrsForForceMergeFunc.
In `@infoschema/builder.go`:
- Around line 808-824: In the backfill-metric cleanup block, evaluate
metrics.DDLHasBackfillMetrics() before computing allColumnPublic and
allIndexPublic so the column/index scans are skipped when no metrics are active.
Preserve the existing cleanup behavior, including partition definition handling,
when backfill metrics are present and both visibility checks pass.
In `@planner/core/planbuilder.go`:
- Around line 809-837: Update fieldNameToColumnName to return immediately unless
fieldName represents the _tidb_rowid column, before calling TableIsView or
TableByName; preserve the existing column-name construction and lookup behavior
for _tidb_rowid, since appendPartitionRowIDWarning is the caller that consumes
this result.
🪄 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: 59d6eddd-1568-4777-bf71-249583313189
📒 Files selected for processing (65)
config/config.goconfig/config_test.goddl/BUILD.bazelddl/backfill_metrics.goddl/backfill_metrics_test.goddl/column.goddl/db_partition_test.goddl/ddl.goddl/ddl_running_jobs.goddl/ddl_running_jobs_test.goddl/delete_range.goddl/index.goddl/index_merge_tmp.goddl/pkdb_force_merge.goddl/pkdb_force_merge_test.goddl/reorg.goddl/schema.goddl/table.godistsql/request_builder.godomain/BUILD.bazeldomain/domain.godomain/domain_test.godomain/infosync/BUILD.bazeldomain/infosync/info.godomain/infosync/pkdb_force_merge.godomain/infosync/pkdb_force_merge_test.godomain/pkdb_force_merge.godomain/pkdb_force_merge_test.goexecutor/distsql.goexecutor/executor_pkg_test.goexecutor/insert_common.goexecutor/update.goexecutor/write_test.goinfoschema/builder.gometa/meta.gometa/meta_test.gometrics/ddl.goowner/manager.goowner/mock.goplanner/core/expression_rewriter.goplanner/core/hf1045_apply_prune_regression_test.goplanner/core/integration_test.goplanner/core/logical_plan_test.goplanner/core/logical_plan_trace_test.goplanner/core/optimizer_test.goplanner/core/planbuilder.goplanner/core/rule_column_pruning.goplanner/core/rule_max_min_eliminate.goplanner/core/rule_result_reorder.goplanner/core/testdata/integration_suite_out.jsonplanner/core/testdata/plan_suite_unexported_out.jsonplanner/core/util.goserver/http_handler.goserver/http_handler_test.goserver/http_status.gosessionctx/variable/BUILD.bazelsessionctx/variable/pkdb_force_merge_test.gosessionctx/variable/sysvar.gosessionctx/variable/sysvar_test.gosessionctx/variable/tidb_vars.gosessionctx/variable/varsutil.gostatistics/handle/ddl_test.gostore/gcworker/gc_worker.gotestkit/result.goutil/memory/tracker.go
| oldColInfo: oldCol, | ||
| newColInfo: newCol, | ||
| metricCounter: metrics.BackfillTotalCounter.WithLabelValues(metrics.GenerateReorgLabel("update_col_rate", reorgInfo.SchemaName, t.Meta().Name.String())), | ||
| metricCounter: getBackfillTotalByTableID(backfillMetricsTableID(reorgInfo, metrics.LblUpdateColRate), metrics.LblUpdateColRate, reorgInfo.SchemaName, t.Meta().Name.String(), oldCol.Name.O), |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Registration uses backfillMetricsTableID, which resolves to the physical partition ID for ActionModifyColumn rate metrics (per ddl/backfill_metrics_test.go's audit case). This is part of a cross-file registration/cleanup table-ID scope mismatch — see the consolidated comment anchored on ddl/reorg.go.
🤖 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 `@ddl/column.go` at line 1205, Update the ActionModifyColumn rate-metric
registration around metricCounter to use the logical table ID expected by the
shared cleanup flow instead of backfillMetricsTableID, which resolves to a
physical partition ID. Align this registration with the table-ID scope validated
by the backfill metrics audit tests and the consolidated reorg handling.
| backfillWorker: newBackfillWorker(jc.ddlJobCtx, sessCtx, id, t, reorgInfo, typeAddIndexMergeTmpWorker), | ||
| index: index, | ||
| jobContext: jc, | ||
| metricCounter: getBackfillTotalByTableID(backfillMetricsTableID(reorgInfo, metrics.LblMergeTmpIdxRate), metrics.LblMergeTmpIdxRate, reorgInfo.SchemaName, t.Meta().Name.String(), indexInfo.Name.O), | ||
| conflictCounter: getBackfillTotalByTableID(backfillMetricsTableID(reorgInfo, metrics.LblMergeTmpIdxRate), metrics.LblMergeTmpIdxRate+"-conflict", reorgInfo.SchemaName, t.Meta().Name.String(), indexInfo.Name.O), |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Registers under PhysicalTableID (per test audit) — part of the same cross-file table-ID scope mismatch. See the consolidated comment anchored on ddl/reorg.go.
🤖 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 `@ddl/index_merge_tmp.go` around lines 143 - 147, Update the metric
registration in the worker initialization around backfillWorker and
conflictCounter so both counters use the logical table ID expected by the reorg
metrics contract rather than PhysicalTableID. Apply the same table-ID selection
consistently to getBackfillTotalByTableID for the normal and conflict metrics,
matching the consolidated handling in reorg.go.
| defaultVals: make([]types.Datum, len(t.WritableCols())), | ||
| rowMap: make(map[int64]types.Datum, len(decodeColMap)), | ||
| metricCounter: metrics.BackfillTotalCounter.WithLabelValues(metrics.GenerateReorgLabel("add_idx_rate", reorgInfo.SchemaName, t.Meta().Name.String())), | ||
| metricCounter: getBackfillTotalByTableID(backfillMetricsTableID(reorgInfo, metrics.LblAddIdxRate), metrics.LblAddIdxRate, reorgInfo.SchemaName, t.Meta().Name.String(), indexInfo.Name.O), |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Registers under PhysicalTableID for ActionAddIndex (per test audit) — part of the cross-file table-ID scope mismatch. See the consolidated comment anchored on ddl/reorg.go.
🤖 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 `@ddl/index.go` at line 1319, Update the ActionAddIndex metricCounter
initialization around getBackfillTotalByTableID and backfillMetricsTableID to
register the backfill metric under the same table-ID scope used by ddl/reorg.go,
rather than PhysicalTableID. Keep the existing metric label, schema, table name,
and index name arguments unchanged, and align the identifier with the other
ActionAddIndex registration paths.
| defaultVals: make([]types.Datum, len(t.WritableCols())), | ||
| rowMap: make(map[int64]types.Datum, len(decodeColMap)), | ||
| metricCounter: metrics.BackfillTotalCounter.WithLabelValues(metrics.GenerateReorgLabel("cleanup_idx_rate", reorgInfo.SchemaName, t.Meta().Name.String())), | ||
| metricCounter: getBackfillTotalByTableID(backfillMetricsTableID(reorgInfo, metrics.LblCleanupIdxRate), metrics.LblCleanupIdxRate, reorgInfo.SchemaName, t.Meta().Name.String(), ""), |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Registers under PhysicalTableID for non-partition-drop actions (per test audit) — part of the same cross-file mismatch. See the consolidated comment anchored on ddl/reorg.go.
🤖 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 `@ddl/index.go` at line 1854, Update the metricCounter registration using
getBackfillTotalByTableID so non-partition-drop actions use the expected table
identifier instead of PhysicalTableID, matching the key used by ddl/reorg.go.
Preserve the existing identifier behavior for partition-drop actions and keep
the current metric label and schema/name arguments unchanged.
|
|
||
| "github.com/pingcap/errors" | ||
| "github.com/pingcap/tidb/ddl/label" | ||
| "github.com/pingcap/tidb/ddl/util" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -A15 'func asyncNotifyEvent' ddl
rg -n 'ddlEventCh\s*(chan|=)' statistics/handleRepository: pingcap/tidb
Length of output: 794
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '560,585p' ddl/ddl.go
printf '\n---\n'
sed -n '1,180p' statistics/handle/handle.go
printf '\n---\n'
sed -n '140,220p' ddl/schema.go
printf '\n---\n'
rg -n "ddlEventCh|asyncNotifyEvent|ActionDropTable|GC" statistics/handle ddl -g '!**/*test.go'Repository: pingcap/tidb
Length of output: 22745
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' statistics/handle/ddl.go
printf '\n---\n'
sed -n '220,280p' statistics/handle/ddl.go
printf '\n---\n'
sed -n '1,140p' statistics/handle/gc.go
printf '\n---\n'
sed -n '430,520p' statistics/handle/handle.go
printf '\n---\n'
sed -n '1230,1275p' statistics/handle/handle.goRepository: pingcap/tidb
Length of output: 15839
Batch drop-schema notifications (ddl/schema.go:250-251)
This sends one best-effort event per table, but ddlEventCh is bounded to 1000 and asyncNotifyEvent drops sends after 10 retries. Large schemas can lose some ActionDropTable notifications, leaving dropped-table stats around until a later cleanup pass.
🤖 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 `@ddl/schema.go` at line 23, Update the drop-schema notification flow that
emits ActionDropTable events so notifications are batched or otherwise delivered
reliably instead of sending one best-effort event per table through
asyncNotifyEvent. Ensure the implementation avoids exhausting the bounded
ddlEventCh and does not lose events after retry limits, while preserving cleanup
for every dropped table.
| func (do *Domain) enableStatsOwner() error { | ||
| if !do.statsOwner.IsOwner() { | ||
| return errors.Trace(do.statsOwner.CampaignOwner()) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether ddl.EnableDDL uses the same unguarded pattern, and whether campaignCancel is nil-safe.
rg -n -A15 'func \(d \*ddl\) EnableDDL' ddl/ddl.go
rg -n 'campaignCancel' owner/manager.goRepository: pingcap/tidb
Length of output: 915
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the owner campaign implementation and the stats-owner caller.
echo '--- owner/manager.go ---'
rg -n -A80 -B20 'func \(m \*Manager\) CampaignOwner|campaignCancel|campaignLoop|IsOwner|campaignOwner' owner/manager.go
echo
echo '--- domain/domain.go ---'
rg -n -A20 -B10 'func \(do \*Domain\) enableStatsOwner' domain/domain.goRepository: pingcap/tidb
Length of output: 10300
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Trace the sysvar hook that calls enableStatsOwner and whether it skips same-value updates.
rg -n -A40 -B20 'enableStatsOwner|stats owner|statsOwner|SetGlobal|sysvar' domain owner -g '*.go'Repository: pingcap/tidb
Length of output: 43240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' owner/manager.go | cat -n | sed -n '1,260p'Repository: pingcap/tidb
Length of output: 9730
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the full CampaignOwner implementation and the surrounding cancellation logic.
sed -n '170,240p' owner/manager.go | cat -nRepository: pingcap/tidb
Length of output: 2807
Guard enableStatsOwner against overlapping campaigns (domain/domain.go:1911-1916) IsOwner() stays false until election succeeds, so repeated calls can start multiple CampaignOwner() goroutines/sessions. Because campaignCancel is shared, later calls also overwrite the cancel func and make shutdown cancel only the latest campaign.
🤖 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 `@domain/domain.go` around lines 1911 - 1916, Update Domain.enableStatsOwner to
prevent overlapping CampaignOwner calls while an election is already in
progress, tracking the active campaign attempt separately from
statsOwner.IsOwner(). Preserve the existing owner fast path, ensure repeated
calls reuse or return the in-flight campaign result, and prevent later attempts
from overwriting the shared campaignCancel used for shutdown.
| func (do *Domain) disableStatsOwner() error { | ||
| do.statsOwner.CampaignCancel() | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first
ast-grep outline domain/domain.go --view expanded || true
ast-grep outline owner/manager.go --view expanded || true
# Read the relevant sections with line numbers
sed -n '1880,1945p' domain/domain.go
printf '\n--- owner/manager.go ---\n'
sed -n '1,260p' owner/manager.go
# Search for campaignCancel initialization and usage
printf '\n--- campaignCancel references ---\n'
rg -n "campaignCancel|CampaignCancel|CampaignOwner\(" domain owner -SRepository: pingcap/tidb
Length of output: 24953
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- owner/mock.go ---'
sed -n '1,180p' owner/mock.go
printf '\n%s\n' '--- DisableStatsOwner references ---'
rg -n "DisableStatsOwner|enableStatsOwner|disableStatsOwner" -S .
printf '\n%s\n' '--- stats owner setup context ---'
sed -n '1930,1975p' domain/domain.goRepository: pingcap/tidb
Length of output: 5442
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- sessionctx/variable/varsutil.go (toggle path) ---'
sed -n '500,545p' sessionctx/variable/varsutil.go
printf '\n%s\n' '--- sessionctx/variable/tidb_vars.go (DisableStatsOwner declaration) ---'
sed -n '1268,1290p' sessionctx/variable/tidb_vars.go
printf '\n%s\n' '--- owner/manager.go (campaign lifecycle) ---'
sed -n '150,220p' owner/manager.goRepository: pingcap/tidb
Length of output: 4782
Guard CampaignCancel in disableStatsOwner
owner.Manager.CampaignCancel() can still be nil here: TiDBEnableStatsOwner may skip CampaignOwner(), but DisableStatsOwner is registered unconditionally and can be reached from the sysvar switch path. Initialize campaignCancel to a no-op or nil-check before calling.
🤖 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 `@domain/domain.go` around lines 1918 - 1921, Update Domain.disableStatsOwner
to safely handle cases where statsOwner or its CampaignCancel callback was not
initialized because CampaignOwner was skipped. Initialize the callback to a
no-op or guard the call before invoking CampaignCancel, while preserving the
existing successful disable behavior.
|
|
||
| if e.memTracker != nil { | ||
| e.memTracker.Reset() | ||
| } else { | ||
| e.memTracker = memory.NewTracker(e.id, -1) | ||
| } | ||
| e.memTracker.AttachTo(e.ctx.GetSessionVars().StmtCtx.MemTracker) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Prevent double-reset of memTracker which discards memory accounting.
e.memTracker is initialized and reset here, and memory is subsequently consumed for the generated key ranges during e.buildTableKeyRanges(). However, Open() then calls e.open() (line 477), which executes the exact same memTracker initialization and reset logic (lines 528-533).
This double-reset will clear the memory tracking accumulated during buildTableKeyRanges(), leading to inaccurate tracking for the executor. Please adjust the initialization logic so that the tracker is only reset once, preserving the key range memory consumption.
🤖 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 `@executor/distsql.go` around lines 458 - 465, Remove the duplicate memTracker
initialization/reset block from the later open() path, while retaining the
single reset or creation before buildTableKeyRanges(). Ensure the existing
AttachTo call uses that same tracker and preserves memory consumed while
generating key ranges.
| // GetBackfillTotalByTableID returns the Counter for the given table ID and type label. | ||
| func GetBackfillTotalByTableID(tableID int64, label, schemaName, tableName, optionalColOrIdxName string) prometheus.Counter { | ||
| typeLabel := GenerateReorgLabel(label, schemaName, tableName, optionalColOrIdxName) | ||
| backfillMetricsRegistry.register(tableID, typeLabel) | ||
| return BackfillTotalCounter.WithLabelValues(typeLabel) | ||
| } | ||
|
|
||
| // GetBackfillProgressByTableID returns the Gauge for the given table ID and type label. | ||
| func GetBackfillProgressByTableID(tableID int64, label, schemaName, tableName, optionalColOrIdxName string) prometheus.Gauge { | ||
| typeLabel := GenerateReorgLabel(label, schemaName, tableName, optionalColOrIdxName) | ||
| backfillMetricsRegistry.register(tableID, typeLabel) | ||
| return BackfillProgressGauge.WithLabelValues(typeLabel) | ||
| } | ||
|
|
||
| // DDLClearBackfillMetrics deletes the backfill metrics registered under a table ID. | ||
| func DDLClearBackfillMetrics(tableID int64) { | ||
| for _, typeLabel := range backfillMetricsRegistry.clear(tableID) { | ||
| BackfillProgressGauge.DeleteLabelValues(typeLabel) | ||
| BackfillTotalCounter.DeleteLabelValues(typeLabel) | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Registration/cleanup table-ID scope must be consistent for DDLClearBackfillMetrics to ever fire.
GetBackfillTotalByTableID/GetBackfillProgressByTableID key the registry purely by the caller-supplied tableID; the underlying Prometheus series itself is keyed only by the generated label string. This means DDLClearBackfillMetrics(tableID) only removes entries that were registered under that exact tableID. See the consolidated comment on ddl/reorg.go (CleanupDDLReorgHandles) for a concrete scenario where registration and cleanup appear to use different ID scopes for the same DDL action.
🤖 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 `@metrics/ddl.go` around lines 224 - 244, Align the ID passed to
GetBackfillTotalByTableID and GetBackfillProgressByTableID with the ID used by
DDLClearBackfillMetrics for the same DDL action. Update the relevant
registration and cleanup paths, including CleanupDDLReorgHandles, so both
consistently use the same table-ID scope and registered labels can be removed.
| func noZeroColumnLayOut(p LogicalPlan) error { | ||
| for _, child := range p.Children() { | ||
| if err := noZeroColumnLayOut(child); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| if p.Schema().Len() == 0 { | ||
| // Some operators do not hold a schema and directly expose their child's schema. | ||
| if len(p.Children()) > 0 && p.Schema() == p.Children()[0].Schema() { | ||
| return nil | ||
| } | ||
| if _, ok := p.(*LogicalTableDual); !ok { | ||
| return errors.Errorf("operator %s has zero row output", p.ExplainID().String()) | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File outline ==\n'
ast-grep outline planner/core/rule_column_pruning.go --view expanded || true
printf '\n== Relevant snippet ==\n'
sed -n '70,130p' planner/core/rule_column_pruning.go | cat -n
printf '\n== Search for Schema() implementations and Clone() usage ==\n'
rg -n 'func \(.*\) Schema\(\)|Schema\(\) \*expression\.Schema|Clone\(\)' planner/core -g '*.go' | sed -n '1,220p'
printf '\n== Search for direct child schema passthrough patterns ==\n'
rg -n 'Children\(\)\[0\]\.Schema\(\)|return .*Schema\(\)' planner/core -g '*.go' | sed -n '1,220p'Repository: pingcap/tidb
Length of output: 29781
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== util.go snippet ==\n'
sed -n '80,110p' planner/core/util.go | cat -n
printf '\n'
sed -n '175,195p' planner/core/util.go | cat -n
printf '\n'
sed -n '225,270p' planner/core/util.go | cat -n
printf '\n== base plan Schema() ==\n'
sed -n '820,855p' planner/core/plan.go | cat -nRepository: pingcap/tidb
Length of output: 4698
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
text = Path('planner/core/rule_column_pruning.go').read_text()
start = text.index('func noZeroColumnLayOut')
end = text.index('func ExprsHasSideEffects')
print(text[start:end])
PYRepository: pingcap/tidb
Length of output: 731
Fix the error text noZeroColumnLayOut checks Schema().Len() == 0, so the panic should say “zero column output,” not “zero row output.” The passthrough check is valid for baseLogicalPlan, which returns the child schema pointer directly.
🤖 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 `@planner/core/rule_column_pruning.go` around lines 97 - 113, Update the error
message in noZeroColumnLayOut to say “zero column output” instead of “zero row
output,” while preserving the existing schema passthrough check and
LogicalTableDual exception.
|
@lichunzhu: The following tests 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. |
10 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
_tidb_rowidhandling and warnings.Tests