Hotfix/hf 1059/v6.5.6 - #70375
Conversation
…umn pruning (pingcap#54609) close pingcap#54213 (cherry picked from commit 9044acb)
close pingcap#54648 (cherry picked from commit 8471e8a)
close pingcap#54777 (cherry picked from commit 60c7e61)
Reviewed-on: https://git.pingcap.net/pingkai/tidb/pulls/715 Reviewed-by: crazycs520 <chenshuang@pingcap.cn>
ti-chi-bot
Bot
added
do-not-merge/invalid-title
release-note-none
|
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. |
|
/ok-to-test |
|
[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 |
|
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. |
|
@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. |
📝 WalkthroughWalkthroughThis change adds DDL backfill metric cleanup, PD force-merge processing, domain-owned statistics control, context-aware subscription shutdown, planner and executor corrections, runtime variables, and related tests and build updates. ChangesStream subscription lifecycle
DDL backfill metrics
PD force merge
Statistics ownership
Planner and executor
Runtime and compatibility updates
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (4)
domain/infosync/pkdb_force_merge.go (1)
102-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
errors.Errorffor the unsupported-endpoint error.The rest of the function wraps errors with
github.com/pingcap/errors.fmt.Errorfcreates an error without a stack trace, so callers lose context.fmtis then only needed for this one call.♻️ Proposed change
- err = fmt.Errorf("PD %s endpoint is unsupported or rejected by precondition", route) + err = errors.Errorf("PD %s endpoint is unsupported or rejected by precondition", route)🤖 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.go` around lines 102 - 108, Replace the fmt.Errorf call in the respBody nil branch of the force-merge request function with github.com/pingcap/errors.Errorf, then remove the fmt import if it is no longer used. Preserve the existing error message and logging behavior.ddl/delete_range.go (1)
275-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOne force-merge reporter is copied into two packages.
reportForceMergeRangesanddoGCForceMergeRangesperform the same four steps: checkvariable.EnableDropTableForceMerge, load the history DDL job, fail when the job is missing, and callinfosync.AddForceMergeRanges. The copies already diverge in the missing-job error type, so future fixes must be applied twice.
ddl/delete_range.go#L275-L293: replace the body with a call to a new exported helper in theddlpackage, for exampleReportForceMergeRangesForGCDeleteRange(ctx, sctx, r, cache), and keepdbterror.ErrDDLJobNotFoundas the single missing-job error.store/gcworker/gc_worker.go#L766-L784: deletedoGCForceMergeRangesand call the sameddlhelper fromdeleteRanges.🤖 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/delete_range.go` around lines 275 - 293, The duplicated force-merge reporting logic must be centralized. In ddl/delete_range.go:275-293, add an exported ddl helper such as ReportForceMergeRangesForGCDeleteRange and have reportForceMergeRanges delegate to it, preserving dbterror.ErrDDLJobNotFound for missing jobs. In store/gcworker/gc_worker.go:766-784, remove doGCForceMergeRanges and update deleteRanges to call the shared ddl helper.config/config.go (1)
506-506: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd doc comments for the new exported symbols.
config/config.go#L506-L506: add a comment that starts withTiDBEnableStatsOwnerand describes the instance configuration state.sessionctx/variable/tidb_vars.go#L1220-L1220: add a comment that starts withEnableDropTableForceMergeand describes the process-level flag.As per coding guidelines, keep exported-symbol doc comments.
🤖 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 `@config/config.go` at line 506, Add Go doc comments for both exported symbols: in config/config.go at lines 506-506, add a comment beginning with TiDBEnableStatsOwner that describes the instance configuration state; in sessionctx/variable/tidb_vars.go at lines 1220-1220, add a comment beginning with EnableDropTableForceMerge that describes the process-level flag.Source: Coding guidelines
ddl/ddl_running_jobs_test.go (1)
98-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for nonterminal
removecalls.This test only removes jobs after
JobStateDone. The new behavior must retainunfinishedSchemawhen a worker releases a nonterminal job. Add an assertion thatremoveclearsallIDs()but keeps a conflicting job unrunnable until the original job becomes done or synced.🤖 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/ddl_running_jobs_test.go` around lines 98 - 110, Extend the removal test around j.remove to cover a nonterminal job state: remove a job before setting it to JobStateDone, assert it is absent from allIDs(), and verify checkRunnable still rejects a conflicting job. Then transition the original job to done or synced and assert the conflicting job becomes runnable, preserving the existing terminal-removal checks.
🤖 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 `@br/pkg/streamhelper/advancer_test.go`:
- Around line 414-419: In TestOwnershipLost and TestSubscriptionPanic, validate
every failpoint.Enable and failpoint.Disable call with require.NoError(t, ...),
including the setup at br/pkg/streamhelper/advancer_test.go lines 414-419 and
the sibling call at lines 436-436. Ensure failures abort the tests before
exercising paths that depend on the failpoints.
- Around line 432-433: The subscription exit callbacks are registered too early,
allowing injected panics to invoke wg.Done more times than the initial count. In
the test setup around TEST_registerCallbackForSubscriptions, inject the panic
failpoints first, then register callbacks immediately before cancellation,
assert that at least one current subscription was registered, and retain the
final wg.Wait behavior.
In `@br/pkg/streamhelper/advancer.go`:
- Around line 628-635: Add a Go doc comment immediately above
CheckpointAdvancer.TEST_registerCallbackForSubscriptions stating that it
registers a callback on the current subscriptions; ensure the comment starts
with TEST_registerCallbackForSubscriptions.
In `@br/pkg/streamhelper/flush_subscriber.go`:
- Around line 264-272: Separate the recovery logic from the onDaemonExit
callback in the deferred cleanup around the subscriber execution. Ensure
recover() handles the original panic before invoking onDaemonExit, so a panic
from the callback cannot prevent recovery; preserve the existing warning and
emitError handling through the recovery delegate.
In `@ddl/ddl.go`:
- Around line 757-759: Update the runningJobs lifecycle in the owner setup: add
a locked runningJobs.reset method that clears the existing object in place, and
call it from the SetRetireOwnerHook callback instead of replacing d.runningJobs.
Ensure this hook is registered before DDL ownership or campaigning is enabled,
so the callback is installed before any ownership transition can occur.
In `@ddl/table.go`:
- Line 814: Update ddl/table.go at lines 814-814 in asyncNotifyEvent to include
oldPartitionIDs while retaining the old logical tableID. Update
statistics/handle/ddl.go at lines 51-55 so truncate cleanup advances metadata
versions for both the logical table ID and every old physical partition ID. Add
static- and dynamic-pruning partitioned truncate cases in
statistics/handle/ddl_test.go at lines 87-105, verifying all old statistics rows
receive a new version.
In `@distsql/request_builder.go`:
- Around line 747-749: Update the memory accounting in the helper around the
memTracker.Consume call to charge all preallocated kv.KeyRange values: use the
total backing-array capacity represented by len(tids) and len(ranges), rather
than only len(ranges), while preserving the existing key-byte accounting.
In `@domain/BUILD.bazel`:
- Line 11: Regenerate domain/BUILD.bazel using make bazel_prepare so the
domain_test target includes pkdb_force_merge_test.go while preserving
shard_count at 25 and adding no dependencies.
In `@domain/pkdb_force_merge.go`:
- Around line 1-13: Replace the headers in domain/pkdb_force_merge.go lines 1-13
and domain/pkdb_force_merge_test.go lines 1-13 with the repository-standard TiDB
copyright and Apache 2.0 header copied from a nearby Go file; make no other
changes.
- Line 48: Call CampaignOwner on the manager returned by Domain.newOwnerManager
in the merge-empty-regions initialization before relying on it for
doMergeEmptyRegions. Use mergeEmptyRegionsOwnerKey for the campaign and preserve
the existing mergeEmptyRegionsOwnerPrompt setup.
In `@executor/insert_common.go`:
- Around line 1201-1207: Update the handle-key duplicate path in the surrounding
insert method to assign the boolean result from removeRow to skip, then exit or
bypass unique-key processing when skip is true before reaching addRecord.
Preserve the existing identical-row behavior for unique-key duplicates, and add
a regression case covering a default clustered primary key.
In `@owner/mock.go`:
- Around line 105-108: Update mockManager.RetireOwner to invoke the stored
retireHook callback before clearing owner, matching ownerManager.RetireOwner
behavior; preserve SetRetireOwnerHook’s callback storage.
In `@planner/core/hf1045_apply_prune_regression_test.go`:
- Line 1: Copy the standard TiDB copyright and Apache 2.0 license header from a
nearby Go source file and add it before the package declaration in the new test
file, leaving the existing package declaration and test code unchanged.
In `@planner/core/integration_test.go`:
- Around line 1933-1945: Update the test around the query with the
SEMI_JOIN_REWRITE hint to assert a stable EXPLAIN or optimizer-trace property
proving the semi-join rewrite was applied, rather than relying only on the
unchanged result row. Preserve the existing result assertion while adding the
rewrite-specific verification using the test’s established inspection mechanism.
In `@sessiontxn/staleread/externalts_test.go`:
- Around line 72-75: Update the insert assertion in the test to verify the error
message contains “only support read-only statement during read-only staleness
transactions” rather than merely checking that an error occurred, covering
validateStatementReadOnlyInStaleness while preserving the existing setup.
In `@store/gcworker/gc_worker.go`:
- Around line 733-740: Update the GC force-merge reporting flow around
doGCForceMergeRanges to disable further PD reporting after the first failure.
Declare forceMergeReportDisabled alongside gcForceMergeTableCache, skip the call
when it is true, and set it to true when doGCForceMergeRanges returns an error
while preserving the existing error log for the initial failure.
In `@util/printer/printer.go`:
- Line 171: Restore the `go:linkname` directive for `buildVersion` in
`util/printer/printer.go` so it is recognized by the linker, or update both
`PrintTiDBInfo` and `GetTiDBInfo` to use `runtime.Version()` consistently with
the non-Go-1.12-aware implementation. Ensure both functions report a populated
`GoVersion`.
---
Nitpick comments:
In `@config/config.go`:
- Line 506: Add Go doc comments for both exported symbols: in config/config.go
at lines 506-506, add a comment beginning with TiDBEnableStatsOwner that
describes the instance configuration state; in sessionctx/variable/tidb_vars.go
at lines 1220-1220, add a comment beginning with EnableDropTableForceMerge that
describes the process-level flag.
In `@ddl/ddl_running_jobs_test.go`:
- Around line 98-110: Extend the removal test around j.remove to cover a
nonterminal job state: remove a job before setting it to JobStateDone, assert it
is absent from allIDs(), and verify checkRunnable still rejects a conflicting
job. Then transition the original job to done or synced and assert the
conflicting job becomes runnable, preserving the existing terminal-removal
checks.
In `@ddl/delete_range.go`:
- Around line 275-293: The duplicated force-merge reporting logic must be
centralized. In ddl/delete_range.go:275-293, add an exported ddl helper such as
ReportForceMergeRangesForGCDeleteRange and have reportForceMergeRanges delegate
to it, preserving dbterror.ErrDDLJobNotFound for missing jobs. In
store/gcworker/gc_worker.go:766-784, remove doGCForceMergeRanges and update
deleteRanges to call the shared ddl helper.
In `@domain/infosync/pkdb_force_merge.go`:
- Around line 102-108: Replace the fmt.Errorf call in the respBody nil branch of
the force-merge request function with github.com/pingcap/errors.Errorf, then
remove the fmt import if it is no longer used. Preserve the existing error
message and logging behavior.
🪄 Autofix
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: dbe784b4-25bc-416d-ae83-88a6d0919e8a
📒 Files selected for processing (74)
br/pkg/streamhelper/BUILD.bazelbr/pkg/streamhelper/advancer.gobr/pkg/streamhelper/advancer_test.gobr/pkg/streamhelper/flush_subscriber.goconfig/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.goddl/util/event.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_trace_test.goplanner/core/optimizer_test.goplanner/core/planbuilder.goplanner/core/rule_build_key_info.goplanner/core/rule_column_pruning.goplanner/core/rule_max_min_eliminate.goplanner/core/rule_result_reorder.goplanner/core/testdata/integration_suite_out.jsonplanner/core/util.goprivilege/privileges/privileges_test.goserver/http_handler.goserver/http_handler_test.goserver/http_status.gosession/session.gosessionctx/variable/BUILD.bazelsessionctx/variable/pkdb_force_merge_test.gosessionctx/variable/session.gosessionctx/variable/sysvar.gosessionctx/variable/tidb_vars.gosessionctx/variable/varsutil.gosessiontxn/staleread/externalts_test.gostatistics/handle/ddl.gostatistics/handle/ddl_test.gostatistics/handle/gc.gostore/gcworker/gc_worker.goutil/memory/tracker.goutil/printer/printer.go
| failpoint.Enable("github.com/pingcap/tidb/br/pkg/streamhelper/subscription.listenOver.aboutToSend", "pause") | ||
| failpoint.Enable("github.com/pingcap/tidb/br/pkg/streamhelper/FlushSubscriber.Clear.timeoutMs", "return(500)") | ||
| wg := new(sync.WaitGroup) | ||
| wg.Add(adv.TEST_registerCallbackForSubscriptions(wg.Done)) | ||
| cancel() | ||
| failpoint.Disable("github.com/pingcap/tidb/br/pkg/streamhelper/subscription.listenOver.aboutToSend") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check failpoint setup errors.
TestOwnershipLost and TestSubscriptionPanic do not validate failpoint.Enable/failpoint.Disable errors. Use assertions such as require.NoError(t, failpoint.Enable(...)) for setup; otherwise the test can pass without exercising the injected shutdown or panic paths.
📍 Affects 1 file
br/pkg/streamhelper/advancer_test.go#L414-L419(this comment)br/pkg/streamhelper/advancer_test.go#L436-L436
🤖 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 `@br/pkg/streamhelper/advancer_test.go` around lines 414 - 419, In
TestOwnershipLost and TestSubscriptionPanic, validate every failpoint.Enable and
failpoint.Disable call with require.NoError(t, ...), including the setup at
br/pkg/streamhelper/advancer_test.go lines 414-419 and the sibling call at lines
436-436. Ensure failures abort the tests before exercising paths that depend on
the failpoints.
| wg := new(sync.WaitGroup) | ||
| wg.Add(adv.TEST_registerCallbackForSubscriptions(wg.Done)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Register exit callbacks after the injected panics.
onDaemonExit runs after every listener exit. The 5*panic failpoint can call wg.Done more times than the initial subscription count. This causes a negative WaitGroup counter or a stuck final wg.Wait().
Move callback registration to immediately before cancellation. Assert that at least one current subscription was registered.
Proposed fix
ctx, cancel := context.WithCancel(context.Background())
+ defer cancel()
env := &testEnv{fakeCluster: c, testCtx: t}
adv := streamhelper.NewCheckpointAdvancer(env)
adv.OnStart(ctx)
adv.OnBecomeOwner(ctx)
- wg := new(sync.WaitGroup)
- wg.Add(adv.TEST_registerCallbackForSubscriptions(wg.Done))
require.NoError(t, adv.OnTick(ctx))
- failpoint.Enable("github.com/pingcap/tidb/br/pkg/streamhelper/subscription.listenOver.aboutToSend", "5*panic")
+ require.NoError(t, failpoint.Enable("github.com/pingcap/tidb/br/pkg/streamhelper/subscription.listenOver.aboutToSend", "5*panic"))
ckpt := c.advanceCheckpoints()
c.flushAll()
cnt := 0
@@
t.Fatalf("After 100 times, the progress cannot be advanced.")
}
}
+ wg := new(sync.WaitGroup)
+ subscriptionCount := adv.TEST_registerCallbackForSubscriptions(wg.Done)
+ if subscriptionCount == 0 {
+ t.Fatal("no subscription callback registered")
+ }
+ wg.Add(subscriptionCount)
cancel()
wg.Wait()📝 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.
| wg := new(sync.WaitGroup) | |
| wg.Add(adv.TEST_registerCallbackForSubscriptions(wg.Done)) | |
| wg := new(sync.WaitGroup) | |
| subscriptionCount := adv.TEST_registerCallbackForSubscriptions(wg.Done) | |
| if subscriptionCount == 0 { | |
| t.Fatal("no subscription callback registered") | |
| } | |
| wg.Add(subscriptionCount) | |
| cancel() | |
| wg.Wait() |
🤖 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 `@br/pkg/streamhelper/advancer_test.go` around lines 432 - 433, The
subscription exit callbacks are registered too early, allowing injected panics
to invoke wg.Done more times than the initial count. In the test setup around
TEST_registerCallbackForSubscriptions, inject the panic failpoints first, then
register callbacks immediately before cancellation, assert that at least one
current subscription was registered, and retain the final wg.Wait behavior.
| func (c *CheckpointAdvancer) TEST_registerCallbackForSubscriptions(f func()) int { | ||
| cnt := 0 | ||
| for _, sub := range c.subscriber.subscriptions { | ||
| sub.onDaemonExit = f | ||
| cnt += 1 | ||
| } | ||
| return cnt | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a doc comment for the exported helper.
TEST_registerCallbackForSubscriptions is exported. Add a comment that starts with its identifier and states that it registers a callback on current subscriptions.
🤖 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 `@br/pkg/streamhelper/advancer.go` around lines 628 - 635, Add a Go doc comment
immediately above CheckpointAdvancer.TEST_registerCallbackForSubscriptions
stating that it registers a callback on the current subscriptions; ensure the
comment starts with TEST_registerCallbackForSubscriptions.
Source: Coding guidelines
| defer func() { | ||
| if s.onDaemonExit != nil { | ||
| s.onDaemonExit() | ||
| } | ||
|
|
||
| if pData := recover(); pData != nil { | ||
| log.Warn("Subscriber paniked.", zap.Uint64("store", storeID), zap.Any("panic-data", pData), zap.Stack("stack")) | ||
| s.emitError(errors.Annotatef(berrors.ErrUnknown, "panic during executing: %v", pData)) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '261,276p' br/pkg/streamhelper/flush_subscriber.goRepository: pingcap/tidb
Length of output: 768
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '130,340p' br/pkg/streamhelper/flush_subscriber.goRepository: pingcap/tidb
Length of output: 6188
🌐 Web query:
Go language specification defer recover panic order of deferred functions panic
💡 Result:
In Go, the interaction between defer, panic, and recover follows a predictable, well-defined sequence governed by the stack-unwinding process [1][2]. Execution Order of Deferred Functions Deferred functions are always executed in Last-In-First-Out (LIFO) order [1]. When a function executes a panic, its normal execution stops immediately [1][3]. The runtime then begins to unwind the stack for the current goroutine, executing all deferred functions associated with that function in reverse order of their registration [1][3]. This process continues up the call stack frame by frame, executing the deferred functions in each frame until the stack is fully unwound or a panic is recovered [1][3]. Role of Panic A panic interrupts the standard control flow [1][2]. When a function calls panic, it stops execution, runs its own deferred functions, and then returns to its caller [1][3]. To the caller, this return behaves as if the caller itself had called panic, triggering the caller's own deferred functions [1][3]. If no deferred function recovers the panic, this continues until the panic reaches the top level of the goroutine, causing the program to crash with a stack trace [1][4][2]. Role of Recover The recover built-in function is the only way to regain control of a panicking goroutine [1][2]. It is effective only when called directly within a deferred function [1][2][3]. 1. If called during normal execution, recover returns nil and has no effect [1][3]. 2. If the goroutine is panicking, a successful call to recover stops the unwinding process [1][2][3]. 3. Once recovered, the panic value is returned, and execution of the current function proceeds normally as if the deferred function had returned [2][3]. Key Implementation Detail For recover to be effective, it must be called directly within the deferred function [2][3]. If a deferred function calls another function that then calls recover, the recovery will not take effect because the "direct caller" requirement is not met [2][3]. [1], [2], [3]
Citations:
- 1: https://go.dev/blog/defer-panic-and-recover
- 2: https://golang.design/under-the-hood/en/part2lang/ch06func/panic/
- 3: https://go.dev/src/builtin/builtin.go?h=panic
- 4: https://go.dev/wiki/PanicAndRecover
🌐 Web query:
Go language specification recover panicking value stored function call defers not panic recovered
💡 Result:
In Go, the built-in recover function is used to regain control of a panicking goroutine [1][2]. For recover to successfully stop a panic and retrieve the panicking value, it must be called directly within a deferred function [1][3]. The Go language specification and runtime enforce specific requirements for recover to be effective [4][3]: 1. Direct Invocation: The recover function must be called directly by the deferred function [3][5]. If recover is called within a function that is subsequently called by the deferred function, it will not be able to recover the panic and will return nil [1][6]. 2. Deferred Context: The call to recover must occur within the body of a deferred function [1][2]. If recover is called during normal execution (when the goroutine is not panicking), it has no effect and returns nil [1][2]. 3. Execution Scope: If the above conditions are met, recover stops the panicking sequence, restores normal execution, and returns the value that was originally passed to the panic function [1][7][2]. A common pitfall is using defer recover directly. In this case, recover is called when the defer statement is registered—not when the deferred function executes—or is treated as the deferred function itself, meaning it is not called directly by the deferred function in the required manner. Consequently, this specific usage returns nil and fails to stop the panic [3][8]. If a panic is not recovered, the panicking sequence continues to propagate up the call stack until all functions in the goroutine have returned, at which point the program terminates with a non-zero exit code and a stack trace [1][7][2].
Citations:
- 1: https://go.dev/src/builtin/builtin.go?h=panic
- 2: https://go.dev/blog/defer-panic-and-recover
- 3: spec: clarify when calling recover stops a panic golang/go#34530
- 4: https://go.dev/ref/spec
- 5: https://www.go101.org/article/panic-and-recover-more.html
- 6: https://golang.design/under-the-hood/en/part2lang/ch06func/panic/
- 7: https://go.dev/wiki/PanicAndRecover
- 8: Documentation: deferring a recover directly is a no-op (spec) golang/go#25815
Recover callback panics before they escape.
In this deferred function, s.onDaemonExit() runs before recover() executes. If the callback panics, recover() runs in a deferred callback called from the first deferred function and cannot recover the original panic. Register onDaemonExit() and recover() in separate deferred functions; recover first if the callback panic should stop unwinding, or run the callback from the recovery delegate.
🤖 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 `@br/pkg/streamhelper/flush_subscriber.go` around lines 264 - 272, Separate the
recovery logic from the onDaemonExit callback in the deferred cleanup around the
subscriber execution. Ensure recover() handles the original panic before
invoking onDaemonExit, so a panic from the callback cannot prevent recovery;
preserve the existing warning and emitError handling through the recovery
delegate.
| d.ownerManager.SetRetireOwnerHook(func() { | ||
| d.runningJobs = newRunningJobs() | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reset the existing runningJobs object instead of replacing its pointer.
The hook runs before ownerManager.RetireOwner clears the election pointer. Workers can still access d.runningJobs while this assignment replaces it. wait4Switch also reads this field without a pointer lock. This creates a data race.
Add a locked runningJobs.reset method and call it from this hook. Register the hook before enabling DDL ownership so the lifecycle callback is installed before campaigning starts.
🤖 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/ddl.go` around lines 757 - 759, Update the runningJobs lifecycle in the
owner setup: add a locked runningJobs.reset method that clears the existing
object in place, and call it from the SetRetireOwnerHook callback instead of
replacing d.runningJobs. Ensure this hook is registered before DDL ownership or
campaigning is enabled, so the callback is installed before any ownership
transition can occur.
| @@ -0,0 +1,73 @@ | |||
| package core_test | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the required license header.
This new Go source file has no standard TiDB copyright and Apache 2.0 license header. Copy the header from a nearby Go file before package core_test.
As per coding guidelines, new Go source files must include the standard TiDB copyright and Apache 2.0 license header copied from a nearby file.
🤖 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/hf1045_apply_prune_regression_test.go` at line 1, Copy the
standard TiDB copyright and Apache 2.0 license header from a nearby Go source
file and add it before the package declaration in the new test file, leaving the
existing package declaration and test code unchanged.
Source: Coding guidelines
| tk.MustQuery(`with c as (select * from t) | ||
| select max(t.a) | ||
| from t | ||
| where exists ( | ||
| select /*+ SEMI_JOIN_REWRITE() */ 1 | ||
| from c | ||
| where c.a < 1 | ||
| ) | ||
| and exists ( | ||
| select 1 | ||
| from c | ||
| where c.a < 2 | ||
| )`).Check(testkit.Rows("1")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the semi-join rewrite result.
The query result remains 1 whether SEMI_JOIN_REWRITE() takes effect or not. Inspect a stable EXPLAIN or optimizer-trace property that proves the rewrite occurred. This test otherwise cannot detect a regression that silently ignores the hint.
🤖 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/integration_test.go` around lines 1933 - 1945, Update the test
around the query with the SEMI_JOIN_REWRITE hint to assert a stable EXPLAIN or
optimizer-trace property proving the semi-join rewrite was applied, rather than
relying only on the unchanged result row. Preserve the existing result assertion
while adding the rewrite-specific verification using the test’s established
inspection mechanism.
| // with tidb_enable_external_ts_read enabled, this session will be readonly | ||
| tk.MustExec("set tidb_enable_external_ts_read=ON") | ||
| _, err := tk.Exec("insert into t values (0)") | ||
| require.Error(t, err) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files =="
git ls-files | rg '(^|/)externalts_test\.go$|sessiontxn/staleread|validateStatementReadOnlyInStaleness' || true
echo "== target file context =="
target="$(git ls-files | rg '(^|/)externalts_test\.go$' | head -n1 || true)"
if [ -n "${target:-}" ]; then
awk 'NR>=1 && NR<=130 {printf "%4d\t%s\n", NR, $0}' "$target"
fi
echo "== relevant searches =="
rg -n "validateStatementReadOnlyInStaleness|read-only staleness transactions|[eE]xternal_ts|tidb_enable_external_ts_read|only support read-only" .Repository: pingcap/tidb
Length of output: 12004
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== session validation implementation =="
sed -n '2270,2325p' session/session.go
echo "== stale transaction error tests nearby =="
sed -n '560,605p' executor/stale_txn_test.goRepository: pingcap/tidb
Length of output: 4107
Assert the expected read-only staleness error.
require.Error accepts any failure. Assert that the rejected insert contains only support read-only statement during read-only staleness transactions so the test covers validateStatementReadOnlyInStaleness.
Proposed test assertion
- require.Error(t, err)
+ require.ErrorContains(t, err, "only support read-only statement during read-only staleness transactions")📝 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.
| // with tidb_enable_external_ts_read enabled, this session will be readonly | |
| tk.MustExec("set tidb_enable_external_ts_read=ON") | |
| _, err := tk.Exec("insert into t values (0)") | |
| require.Error(t, err) | |
| // with tidb_enable_external_ts_read enabled, this session will be readonly | |
| tk.MustExec("set tidb_enable_external_ts_read=ON") | |
| _, err := tk.Exec("insert into t values (0)") | |
| require.ErrorContains(t, err, "only support read-only statement during read-only staleness transactions") |
🤖 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 `@sessiontxn/staleread/externalts_test.go` around lines 72 - 75, Update the
insert assertion in the test to verify the error message contains “only support
read-only statement during read-only staleness transactions” rather than merely
checking that an error occurred, covering validateStatementReadOnlyInStaleness
while preserving the existing setup.
| if err := w.doGCForceMergeRanges(ctx, se, r, gcForceMergeTableCache); err != nil { | ||
| logutil.Logger(ctx).Error("[gc worker] report force merge ranges failed on range", | ||
| zap.String("uuid", w.uuid), | ||
| zap.Int64("jobID", r.JobID), | ||
| zap.Int64("elementID", r.ElementID), | ||
| zap.Error(err)) | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A PD outage can stall GC because every task pays the full force-merge timeout.
doGCForceMergeRanges runs a synchronous HTTP call to PD for each completed delete-range task. AddForceMergeRanges uses forceMergePDRequestTimeout (20s) per PD address, and it retries every configured address. If PD does not answer, deleteRanges adds that delay once per task and only logs the failure. A GC run with many pending ranges then takes far longer than before, which delays the remaining GC stages.
Short-circuit reporting for the rest of the run after the first failure, or count consecutive failures and stop calling PD.
🛡️ Proposed guard
- if err := w.doGCForceMergeRanges(ctx, se, r, gcForceMergeTableCache); err != nil {
- logutil.Logger(ctx).Error("[gc worker] report force merge ranges failed on range",
- zap.String("uuid", w.uuid),
- zap.Int64("jobID", r.JobID),
- zap.Int64("elementID", r.ElementID),
- zap.Error(err))
- }
+ if !forceMergeReportDisabled {
+ if err := w.doGCForceMergeRanges(ctx, se, r, gcForceMergeTableCache); err != nil {
+ logutil.Logger(ctx).Error("[gc worker] report force merge ranges failed on range",
+ zap.String("uuid", w.uuid),
+ zap.Int64("jobID", r.JobID),
+ zap.Int64("elementID", r.ElementID),
+ zap.Error(err))
+ // Stop reporting for the remainder of this run so a PD outage
+ // does not add the PD request timeout to every task.
+ forceMergeReportDisabled = true
+ }
+ }Declare forceMergeReportDisabled := false next to gcForceMergeTableCache.
🤖 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 `@store/gcworker/gc_worker.go` around lines 733 - 740, Update the GC
force-merge reporting flow around doGCForceMergeRanges to disable further PD
reporting after the first failure. Declare forceMergeReportDisabled alongside
gcForceMergeTableCache, skip the call when it is true, and set it to true when
doGCForceMergeRanges returns an error while preserving the existing error log
for the initial failure.
| } | ||
|
|
||
| //go:linkname buildVersion runtime.buildVersion | ||
| // go:linkname buildVersion runtime.buildVersion |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 '\bbuildVersion\b|go:linkname|runtime\.Version\(\)' --glob '*.go' .
rg -n 'buildVersion\s*=' --glob '*.go' . || trueRepository: pingcap/tidb
Length of output: 7746
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- util/printer/printer.go around buildVersion references ---\n'
sed -n '1,80p;160,176p' util/printer/printer.go
printf '\n--- all buildVersion references (not limited to go files) ---\n'
rg -n -C 3 '\bbuildVersion\b' . || true
printf '\n--- package-private variable assignment patterns for buildVersion ---\n'
rg -n '^\s*(\w+\s*)?buildVersion\b\s*[:=]|buildVersion\s*\+=' --glob '*.go' . || true
printf '\n--- go tool availability and directive behavior probe ---\n'
command -v go || true
if command -v go >/dev/null 2>&1; then
go version
tmpdir="$(mktemp -d)"
cat > "$tmpdir/linkname.go" <<'EOF'
package main
//go:linkname buildVersion runtime.buildVersion
var buildVersion string
func main() {
print(buildVersion)
}
EOF
cat > "$tmpdir/lazymo" <<'EOF'
package main
// go:linkname buildVersion runtime.buildVersion
var buildVersion string
func main() {
println(buildVersion, ":", buildVersion)
}
EOF
printf 'linkname-variadic call:\n'
go run -o /dev/null "$tmpdir/linkname.go" 2>&1 || true
printf '\nlazymo:\n'
go run -o /dev/null "$tmpdir/lazymo" 2>&1 || true
fiRepository: pingcap/tidb
Length of output: 241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- util/printer/printer.go around buildVersion references ---'
sed -n '1,80p;160,176p' util/printer/printer.go
printf '%s\n' ''
printf '%s\n' '--- all buildVersion references (not limited to go files) ---'
rg -n -C 3 '\bbuildVersion\b' . || true
printf '%s\n' ''
printf '%s\n' '--- package-private variable assignment patterns for buildVersion ---'
rg -n '^\s*(\w+\s*)?buildVersion\b\s*[:=]|buildVersion\s*\+=' --glob '*.go' . || true
printf '%s\n' ''
printf '%s\n' '--- go tool availability and directive behavior probe ---'
command -v go || true
if command -v go >/dev/null 2>&1; then
go version
tmpdir="$(mktemp -d)"
cat > "$tmpdir/linkname.go" <<'EOF'
package main
//go:linkname buildVersion runtime.buildVersion
var buildVersion string
func main() {
print(buildVersion)
}
EOF
cat > "$tmpdir/lazymo" <<'EOF'
package main
// go:linkname buildVersion runtime.buildVersion
var buildVersion string
func main() {
println(buildVersion, ":", buildVersion)
}
EOF
printf '%s\n' 'linkname variadic call:'
go run -o /dev/null "$tmpdir/linkname.go" 2>&1 || true
printf '%s\n' ''
printf '%s\n' 'lazy command:'
go run -o /dev/null "$tmpdir/lazymo" 2>&1 || true
fiRepository: pingcap/tidb
Length of output: 5038
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
cat > "$tmpdir/linkname.go" <<'EOF'
package main
//go:linkname buildVersion runtime.buildVersion
var buildVersion string
func main() {
println("valid:", buildVersion, ":", buildVersion)
}
EOF
cat > "$tmpdir/lazy.go" <<'EOF'
package main
// go:linkname runtime.buildVersion
var buildVersion string
func main() {
println("lazy:", buildVersion, ":", buildVersion)
}
EOF
printf '%s\n' 'valid linkname:'
if go run "$tmpdir/linkname.go" >/tmp/linkname.stdout 2>/tmp/linkname.stderr; then
cat /tmp/linkname.stdout
else
cat /tmp/linkname.stderr
fi
printf '%s\n' ''
printf '%s\n' 'lazy directive:'
if go run "$tmpdir/lazy.go" >/tmp/lazy.stdout 2>/tmp/lazy.stderr; then
cat /tmp/lazy.stdout
else
cat /tmp/lazy.stderr
fiRepository: pingcap/tidb
Length of output: 305
Restore the linker variable assignment.
// go:linkname buildVersion runtime.buildVersion is parsed as a normal comment. No other file assigns buildVersion, so both PrintTiDBInfo and GetTiDBInfo report an empty GoVersion. Restore the directive without the space, or replace the two call sites with runtime.Version() as the non-Go-1.12-aware util/printer.go already does.
🤖 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 `@util/printer/printer.go` at line 171, Restore the `go:linkname` directive for
`buildVersion` in `util/printer/printer.go` so it is recognized by the linker,
or update both `PrintTiDBInfo` and `GetTiDBInfo` to use `runtime.Version()`
consistently with the non-Go-1.12-aware implementation. Ensure both functions
report a populated `GoVersion`.
|
/ok-to-test |
ti-chi-bot
Bot
added
ok-to-test
|
@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. |
13 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
tidb_enable_stats_ownerandtidb_enable_drop_table_force_mergesettings.sql_require_primary_keychanges to authorized administrators.