planner: Skip plan cache regeneration if binding exists#67134
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note-none
|
@terry1purcell I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
sig/planner
|
Hi @terry1purcell. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds sysvar Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Planner as Planner
participant BindingSvc as "Bindings"
participant PlanCache as "PlanCache"
participant StatsSvc as "Stats"
participant CacheStore as "CacheStore"
Client->>Planner: PREPARE / EXECUTE statement
Planner->>BindingSvc: check for matched binding
BindingSvc-->>Planner: binding or none
Planner->>PlanCache: NewPlanCacheKey(binding, SessionVars...)
Note over PlanCache: includes statsVerHash only if\nPlanCacheInvalidationOnFreshStats && (binding=="" || !PlanCacheSkipStatsOnBinding)
PlanCache->>CacheStore: lookup key
CacheStore-->>PlanCache: hit / miss
alt ANALYZE occurs
StatsSvc->>PlanCache: update stats version
PlanCache->>CacheStore: invalidate entries depending on key composition
end
PlanCache-->>Planner: plan (from cache or rebuilt)
Planner-->>Client: execute / return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🧹 Nitpick comments (1)
pkg/planner/core/plan_cache_utils.go (1)
384-387: Deduplicate the stats-hash gate to prevent hash-length/content drift.The same condition is repeated in two places. Extracting it once makes future edits safer.
♻️ Proposed refactor
- if vars.GetSessionVars().PlanCacheInvalidationOnFreshStats && (binding == "" || !vars.GetSessionVars().PlanCacheSkipStatsOnBinding) { + includeStatsVerHash := vars.PlanCacheInvalidationOnFreshStats && + (binding == "" || !vars.PlanCacheSkipStatsOnBinding) + if includeStatsVerHash { // statsVerHash: skipped when a binding is matched and PlanCacheSkipStatsOnBinding is on, // because a binding pins the plan via hints so stats changes cannot alter the chosen plan. hashLen += 8 } @@ - if sctx.GetSessionVars().PlanCacheInvalidationOnFreshStats && (binding == "" || !sctx.GetSessionVars().PlanCacheSkipStatsOnBinding) { + if includeStatsVerHash { var statsVerHash uint64 for _, t := range stmt.tables { statsVerHash += getLatestVersionFromStatsTable(sctx, t.Meta(), t.Meta().ID) // use '+' as the hash function for simplicity } hash = codec.EncodeUint(hash, statsVerHash) }Also applies to: 459-462
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/plan_cache_utils.go` around lines 384 - 387, The repeated condition checking vars.GetSessionVars().PlanCacheInvalidationOnFreshStats && (binding == "" || !vars.GetSessionVars().PlanCacheSkipStatsOnBinding) should be extracted into a single local boolean (e.g., shouldIncludeStatsHash) near the top of the function in pkg/planner/core/plan_cache_utils.go and then used in both spots (the current hashLen += 8 branch and the other occurrence around lines 459-462) to avoid drift; update references to use that boolean when deciding to include the stats hash and remove the duplicated condition expressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/session/upgrade_def.go`:
- Around line 2082-2084: Add automated tests to validate the new
tidb_plan_cache_skip_stats_on_binding variable set by upgradeToVer256: create a
bootstrap upgrade unit test (following the TestUpgradeWithAnalyzeColumnOptions
pattern) that runs upgradeToVer256 and asserts the global variable initialized
by initGlobalVariableIfNotExists(vardef.TiDBPlanCacheSkipStatsOnBinding,
vardef.On) exists and is ON; additionally add SQL integration tests that
exercise plan cache + binding + fresh-stats scenarios with the variable enabled
(create bindings, generate fresh stats, and assert the planner/plan cache
behavior changes expected when tidb_plan_cache_skip_stats_on_binding is ON).
---
Nitpick comments:
In `@pkg/planner/core/plan_cache_utils.go`:
- Around line 384-387: The repeated condition checking
vars.GetSessionVars().PlanCacheInvalidationOnFreshStats && (binding == "" ||
!vars.GetSessionVars().PlanCacheSkipStatsOnBinding) should be extracted into a
single local boolean (e.g., shouldIncludeStatsHash) near the top of the function
in pkg/planner/core/plan_cache_utils.go and then used in both spots (the current
hashLen += 8 branch and the other occurrence around lines 459-462) to avoid
drift; update references to use that boolean when deciding to include the stats
hash and remove the duplicated condition expressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 713b2bc7-ac0f-4e0d-a59f-b8d74af78262
📒 Files selected for processing (5)
pkg/planner/core/plan_cache_utils.gopkg/session/upgrade_def.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/sysvar.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #67134 +/- ##
================================================
+ Coverage 77.8320% 78.9355% +1.1034%
================================================
Files 2023 1960 -63
Lines 556131 544623 -11508
================================================
- Hits 432848 429901 -2947
+ Misses 121539 113696 -7843
+ Partials 1744 1026 -718
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
size/L
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integrationtest/t/planner/core/plan_cache.test (1)
1659-1660: Assert the default oftidb_plan_cache_skip_stats_on_bindingtoo.This block only proves the explicit
ON/OFFpaths. Sincepkg/sessionctx/variable/sysvar.go:3409-3412gives the new switch an explicit default, a bad default would still leave this scenario green. Please add one assertion around theDEFAULTreset as well.Suggested addition
set tidb_plan_cache_invalidation_on_fresh_stats = DEFAULT; set tidb_plan_cache_skip_stats_on_binding = DEFAULT; +select @@session.tidb_plan_cache_skip_stats_on_binding;Also applies to: 1686-1687
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integrationtest/t/planner/core/plan_cache.test` around lines 1659 - 1660, Add an assertion that verifies the DEFAULT value for tidb_plan_cache_skip_stats_on_binding: after toggling ON/OFF and before finishing the test add a statement that sets tidb_plan_cache_skip_stats_on_binding = DEFAULT (or RESET it) and then query the variable and assert it equals the declared default for tidb_plan_cache_skip_stats_on_binding (the same default defined in pkg/sessionctx/variable/sysvar.go). Use the same test harness helpers/assert style used for the existing ON/OFF checks so the new assertion mirrors the other expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/integrationtest/t/planner/core/plan_cache.test`:
- Around line 1659-1660: Add an assertion that verifies the DEFAULT value for
tidb_plan_cache_skip_stats_on_binding: after toggling ON/OFF and before
finishing the test add a statement that sets
tidb_plan_cache_skip_stats_on_binding = DEFAULT (or RESET it) and then query the
variable and assert it equals the declared default for
tidb_plan_cache_skip_stats_on_binding (the same default defined in
pkg/sessionctx/variable/sysvar.go). Use the same test harness helpers/assert
style used for the existing ON/OFF checks so the new assertion mirrors the other
expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fa16cb07-04b7-4b78-82d8-c088a09986b5
📒 Files selected for processing (3)
br/pkg/restore/snap_client/systable_restore_test.gotests/integrationtest/r/planner/core/plan_cache.resulttests/integrationtest/t/planner/core/plan_cache.test
|
/retest-required |
terry1purcell
changed the title
There was a problem hiding this comment.
Pull request overview
This PR introduces a new session/global switch to allow prepared plan cache entries to remain reusable across statistics updates when a SQL binding is matched, reducing cache invalidations triggered by ANALYZE for bound queries (issue #67335).
Changes:
- Add system variable
tidb_plan_cache_skip_stats_on_binding(default ON) and bootstrap/upgrade logic to initialize it. - Update plan-cache key generation to optionally exclude the stats-version component when a binding is matched and the new variable is enabled.
- Add regression tests (planner integration test + Go test + bootstrap upgrade test) and adjust Bazel sharding / bootstrap version expectations.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
pkg/planner/core/plan_cache_utils.go |
Skip stats-version hashing in plan cache key when a binding is matched and the new sysvar is enabled. |
pkg/sessionctx/variable/sysvar.go |
Register new bool sysvar and wire it to SessionVars. |
pkg/sessionctx/variable/session.go |
Add SessionVars.PlanCacheSkipStatsOnBinding field. |
pkg/sessionctx/vardef/tidb_vars.go |
Add sysvar name constant and default value. |
pkg/session/upgrade_def.go |
Bump bootstrap version and add upgrade step to initialize the new sysvar. |
pkg/session/test/bootstraptest/bootstrap_upgrade_test.go |
Add upgrade test ensuring sysvar is initialized on upgrade from v255. |
tests/integrationtest/t/planner/core/plan_cache.test |
Add integration test coverage for binding + stats invalidation behavior. |
tests/integrationtest/r/planner/core/plan_cache.result |
Update expected output for the new integration test. |
pkg/session/test/bootstraptest/BUILD.bazel |
Increase shard count for test distribution. |
pkg/planner/core/casetest/plancache/plan_cache_suite_test.go |
Add unit test covering binding + stats invalidation skip behavior. |
pkg/planner/core/casetest/plancache/BUILD.bazel |
Increase shard count for test distribution. |
br/pkg/restore/snap_client/systable_restore_test.go |
Update expected CurrentBootstrapVersion to 256. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
[LGTM Timeline notifier]Timeline:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BornChanger, hawkingrei, qw4990, yudongusa The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest-required |
4 similar comments
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
What problem does this PR solve?
Issue Number: close #67335
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
Chores
Tests