planner: support use_plan_cache hint and binding under hint_only plan cache strategy#67535
Conversation
ti-chi-bot
Bot
added
the
release-note-none
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
sig/planner
|
Hi @qw4990. 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. |
📝 WalkthroughWalkthroughIntroduce a Changes
Sequence DiagramssequenceDiagram
participant Client
participant Session as SessionVars
participant Optimizer
participant Binding as BindInfo
participant Cache as PlanCache
Client->>Session: Execute SQL (strategy=hint_only)
Session->>Optimizer: Provide stmt + strategy
Optimizer->>Optimizer: Check AST for use_plan_cache hint
alt hint present in AST
Optimizer->>Cache: newPlanCacheKeyWithMatchedBinding(...)
Cache->>Optimizer: Lookup plan
else hint absent
Optimizer->>Binding: MatchSQLBindingWithCache(...)
Binding->>Optimizer: matchedBinding (may contain hint)
alt matched binding has use_plan_cache
Optimizer->>Cache: newPlanCacheKeyWithMatchedBinding(...)
Cache->>Optimizer: Lookup plan
else no hint anywhere
Optimizer->>Session: WarnSkipPlanCache("hint_only no hint")
Optimizer->>Optimizer: Generate plan without cache
end
end
Optimizer->>Client: Return plan/result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
pkg/planner/core/plan_cache_utils.go (1)
315-330: Consider splitting “binding matched” from “binding match already computed” in helper args.At Line 324,
bindingMatchedcurrently controls both rematch behavior and match-result semantics. That contract is easy to misuse at call sites and can reintroduce duplicate matching.♻️ Suggested refactor
-func newPlanCacheKeyWithMatchedBinding( +func newPlanCacheKeyWithMatchedBinding( sctx sessionctx.Context, stmt *PlanCacheStmt, matchedBinding *bindinfo.Binding, - bindingMatched bool, + bindingMatchComputed bool, + bindingMatched bool, ) (key, binding string, cacheable bool, reason string, err error) { - if !bindingMatched && stmt.PreparedAst != nil { + if !bindingMatchComputed && stmt.PreparedAst != nil { matchedBinding, bindingMatched, _ = bindinfo.MatchSQLBindingWithCache(sctx, stmt.PreparedAst.Stmt, &stmt.BindingInfo) } if bindingMatched && matchedBinding != nil { binding = matchedBinding.BindSQL }🤖 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 315 - 330, The helper newPlanCacheKeyWithMatchedBinding conflates "should attempt rematch" with the "bindingMatched result" via the bindingMatched parameter, causing misuse and duplicate matching; change the function signature to accept two distinct booleans (e.g., rematch bool and bindingMatched bool or preMatched bool and doRematch bool), update the internal logic to only call bindinfo.MatchSQLBindingWithCache when the rematch/doRematch flag is true, and treat bindingMatched strictly as the match-result to decide setting binding = matchedBinding.BindSQL; then propagate this new signature to all callers (including the wrapper newPlanCacheKey) and update their call sites to pass the appropriate rematch flag versus the prior match-result so rematching behavior is explicit and no duplicate matching occurs.pkg/planner/core/plan_cache.go (1)
63-73: Consider centralizing thehint_onlygate.This file now has its own helper and reason string for the same
use_plan_cache()/binding policy thatpkg/planner/optimize.goalready enforces for the non-prepared entry point. Pulling that predicate/reason into one shared helper would make future plan-cache-hint changes less likely to drift between the two paths.As per coding guidelines "Code SHOULD remain maintainable for future readers with basic TiDB familiarity, including readers who are not experts in the specific subsystem/feature."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/plan_cache.go` around lines 63 - 73, The new helper containUsePlanCacheHintInPreparedSQLOrBinding and planCacheHintOnlyNoHintReason duplicate the same "hint_only" predicate used in the non-prepared path; extract that predicate and its reason string into a single shared helper (e.g., Move or create a centralized function like IsUsePlanCacheAllowedOrReason/GetUsePlanCacheHintReason) and replace the local planCacheHintOnlyNoHintReason and containUsePlanCacheHintInPreparedSQLOrBinding with calls to that shared helper from both this file and the code in optimize.go; ensure the shared helper accepts a PlanCacheStmt, bindinfo.Binding, and bindingMatched bool and returns the boolean (and/or reason string) so both prepared and non-prepared entry points use the exact same logic.tests/integrationtest/r/planner/core/plan_cache.result (1)
2737-2738: Add one post-drop execution before resettinghint_only.These blocks prove that a binding can turn caching on, but they stop before checking the inverse transition. Please keep
tidb_plan_cache_strategy='hint_only'for one more execute/select afterdrop binding; otherwise a stale binding-match cache in the prepared or non-prepared path would still pass here.Also applies to: 2763-2765
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integrationtest/r/planner/core/plan_cache.result` around lines 2737 - 2738, After the "drop binding for select * from t where a=1;" step, run one more execute/select of the same statement to validate the transition before changing plan cache strategy back to default; specifically, insert an extra "select * from t where a=1;" (or the matching prepared execute) between the "drop binding for select * from t where a=1;" and "set tidb_plan_cache_strategy = default;" lines. Do the same fix for the analogous block around the "drop binding" at the 2763-2765 area so each drop is followed by one more execute/select before resetting tidb_plan_cache_strategy.pkg/util/hint/hint_processor.go (1)
132-132: Optional hardening: normalizehintNameat API boundary.
HintName.Lis lowercase; normalizing input once would make this exported function more robust for future callers.♻️ Suggested tweak
func ContainTableHintInStmtNode(node ast.Node, hintName string) bool { + hintName = strings.ToLower(hintName) switch x := node.(type) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/hint/hint_processor.go` at line 132, The exported function ContainTableHintInStmtNode should normalize the incoming hintName at the API boundary because HintName.L is stored lowercase; update ContainTableHintInStmtNode to canonicalize hintName (e.g., to lower-case or the same normalization used by HintName.L) before any comparisons so callers passing different-cased strings still match reliably.tests/integrationtest/t/planner/core/plan_cache.test (1)
1735-1736: Consider adding post-drop bindingassertions to lock in fallback behavior.A quick re-execution after dropping the binding would ensure cache gating reverts immediately when the hint source is removed.
🧪 Suggested additions
drop binding for select * from t where a=1; +execute st using `@a`; +execute st using `@a`; +select @@last_plan_from_binding, @@last_plan_from_cache; set tidb_plan_cache_strategy = default;drop binding for select * from t where a=1; +select * from t where a = 1; +select * from t where a = 1; +select @@last_plan_from_binding, @@last_plan_from_cache; set tidb_enable_non_prepared_plan_cache = default; set tidb_plan_cache_strategy = default;Also applies to: 1751-1753
🤖 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 1735 - 1736, After the "drop binding for select * from t where a=1;" and "set tidb_plan_cache_strategy = default;" statements, re-run the same query ("select * from t where a=1") and add assertions that verify the binding no longer gates the plan/cache (e.g., assert the plan fingerprint or that the plan_cache does not contain the binding and the execution uses the default strategy), ensuring the test immediately observes fallback behavior; apply the same post-drop re-execution and assertions at the other occurrence around the 1751-1753 block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/planner/core/plan_cache_utils.go`:
- Around line 315-330: The helper newPlanCacheKeyWithMatchedBinding conflates
"should attempt rematch" with the "bindingMatched result" via the bindingMatched
parameter, causing misuse and duplicate matching; change the function signature
to accept two distinct booleans (e.g., rematch bool and bindingMatched bool or
preMatched bool and doRematch bool), update the internal logic to only call
bindinfo.MatchSQLBindingWithCache when the rematch/doRematch flag is true, and
treat bindingMatched strictly as the match-result to decide setting binding =
matchedBinding.BindSQL; then propagate this new signature to all callers
(including the wrapper newPlanCacheKey) and update their call sites to pass the
appropriate rematch flag versus the prior match-result so rematching behavior is
explicit and no duplicate matching occurs.
In `@pkg/planner/core/plan_cache.go`:
- Around line 63-73: The new helper
containUsePlanCacheHintInPreparedSQLOrBinding and planCacheHintOnlyNoHintReason
duplicate the same "hint_only" predicate used in the non-prepared path; extract
that predicate and its reason string into a single shared helper (e.g., Move or
create a centralized function like
IsUsePlanCacheAllowedOrReason/GetUsePlanCacheHintReason) and replace the local
planCacheHintOnlyNoHintReason and containUsePlanCacheHintInPreparedSQLOrBinding
with calls to that shared helper from both this file and the code in
optimize.go; ensure the shared helper accepts a PlanCacheStmt, bindinfo.Binding,
and bindingMatched bool and returns the boolean (and/or reason string) so both
prepared and non-prepared entry points use the exact same logic.
In `@pkg/util/hint/hint_processor.go`:
- Line 132: The exported function ContainTableHintInStmtNode should normalize
the incoming hintName at the API boundary because HintName.L is stored
lowercase; update ContainTableHintInStmtNode to canonicalize hintName (e.g., to
lower-case or the same normalization used by HintName.L) before any comparisons
so callers passing different-cased strings still match reliably.
In `@tests/integrationtest/r/planner/core/plan_cache.result`:
- Around line 2737-2738: After the "drop binding for select * from t where a=1;"
step, run one more execute/select of the same statement to validate the
transition before changing plan cache strategy back to default; specifically,
insert an extra "select * from t where a=1;" (or the matching prepared execute)
between the "drop binding for select * from t where a=1;" and "set
tidb_plan_cache_strategy = default;" lines. Do the same fix for the analogous
block around the "drop binding" at the 2763-2765 area so each drop is followed
by one more execute/select before resetting tidb_plan_cache_strategy.
In `@tests/integrationtest/t/planner/core/plan_cache.test`:
- Around line 1735-1736: After the "drop binding for select * from t where a=1;"
and "set tidb_plan_cache_strategy = default;" statements, re-run the same query
("select * from t where a=1") and add assertions that verify the binding no
longer gates the plan/cache (e.g., assert the plan fingerprint or that the
plan_cache does not contain the binding and the execution uses the default
strategy), ensuring the test immediately observes fallback behavior; apply the
same post-drop re-execution and assertions at the other occurrence around the
1751-1753 block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2047313c-f69c-4612-a680-3f3b9c1befce
📒 Files selected for processing (11)
pkg/parser/ast/misc.gopkg/planner/core/plan_cache.gopkg/planner/core/plan_cache_utils.gopkg/planner/optimize.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/sysvar.gopkg/util/hint/hint.gopkg/util/hint/hint_processor.gotests/integrationtest/r/planner/core/plan_cache.resulttests/integrationtest/t/planner/core/plan_cache.test
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67535 +/- ##
================================================
+ Coverage 77.5423% 77.8731% +0.3307%
================================================
Files 1962 1965 +3
Lines 544528 551311 +6783
================================================
+ Hits 422240 429323 +7083
- Misses 121478 121966 +488
+ Partials 810 22 -788
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/test unit-test |
|
@qw4990: PRs from untrusted users cannot be marked as trusted with 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 |
|
/retest |
1 similar comment
|
/retest |
| if sctx.GetSessionVars().PlanCacheStrategy == vardef.TiDBPlanCacheStrategyHintOnly && | ||
| !containUsePlanCacheHintInSQLOrBinding(sctx, stmt) { | ||
| if !isExplain && stmtCtx.InExplainStmt && stmtCtx.ExplainFormat == types.ExplainFormatPlanCache { | ||
| stmtCtx.AppendWarning(errors.NewNoStackErrorf("skip non-prepared plan-cache: %s", nonPreparedPlanCacheHintOnlyNoHintReason)) |
There was a problem hiding this comment.
can we add warning test about this？
There was a problem hiding this comment.
Sure, the new test case has been added:
| } | ||
| if cacheEnabled && stmt.UncacheableReason == "" && | ||
| sessVars.PlanCacheStrategy == vardef.TiDBPlanCacheStrategyHintOnly && | ||
| !stmt.HasUsePlanCacheHint { |
There was a problem hiding this comment.
!stmt.HasUsePlanCacheHint may overlap with !containUsePlanCacheHintInPreparedSQLOrBinding check below？
There was a problem hiding this comment.
No, stmt.HasUsePlanCacheHint is from the statement directly, but containUsePlanCacheHintInPreparedSQLOrBinding is from the binding.
prepare st1 from "select /*+ use_plan_cache() */ * from t1";
prepare st2 from "select * from t2";
create global binding using select /*+ use_plan_cache() */ * from t2
In the case above, st1.HasUsePlanCacheHint is true, but st2.HashUsePlanCacheHint is false, st2 has to get the use_plan_cache from binding, which is what containUsePlanCacheHintInPreparedSQLOrBinding for.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/plan_cache.go (1)
66-74: Consider centralizing theuse_plan_cache()detection.This file now has its own binding-aware
hint_onlycheck, while the non-prepared path enforces the same rule inpkg/planner/optimize.go. Moving that logic behind one helper would make it easier to keep the prepared and non-prepared cache paths in sync as hint sources evolve.As per coding guidelines, "Code SHOULD remain maintainable for future readers with basic TiDB familiarity, including readers who are not experts in the specific subsystem/feature."
Also applies to: 226-236
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/plan_cache.go` around lines 66 - 74, Extract the duplicated detection logic into a single helper function (e.g., IsUsePlanCacheHintPresent) that accepts the prepared statement (PlanCacheStmt), optional bindinfo.Binding and bindingMatched flag and returns whether use_plan_cache() is present from either stmt.HasUsePlanCacheHint or the matchedBinding.Hint. Replace the body of containUsePlanCacheHintInPreparedSQLOrBinding and the corresponding check in pkg/planner/optimize.go (and the other occurrence at lines ~226-236) to call this new helper so both prepared and non-prepared code paths share the same source of truth and preserve current semantics (null checks and Hint.ContainTableHint(hint.HintUsePlanCache)). Ensure the helper is placed in a package accessible to both call sites and keep parameter names and null-safety consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/planner/core/plan_cache.go`:
- Around line 66-74: Extract the duplicated detection logic into a single helper
function (e.g., IsUsePlanCacheHintPresent) that accepts the prepared statement
(PlanCacheStmt), optional bindinfo.Binding and bindingMatched flag and returns
whether use_plan_cache() is present from either stmt.HasUsePlanCacheHint or the
matchedBinding.Hint. Replace the body of
containUsePlanCacheHintInPreparedSQLOrBinding and the corresponding check in
pkg/planner/optimize.go (and the other occurrence at lines ~226-236) to call
this new helper so both prepared and non-prepared code paths share the same
source of truth and preserve current semantics (null checks and
Hint.ContainTableHint(hint.HintUsePlanCache)). Ensure the helper is placed in a
package accessible to both call sites and keep parameter names and null-safety
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9bc5b9ff-5a66-4032-863c-fc53a3a2c279
📒 Files selected for processing (4)
pkg/planner/core/plan_cache.gopkg/planner/optimize.gotests/integrationtest/r/planner/core/plan_cache.resulttests/integrationtest/t/planner/core/plan_cache.test
✅ Files skipped from review due to trivial changes (2)
- pkg/planner/optimize.go
- tests/integrationtest/t/planner/core/plan_cache.test
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integrationtest/r/planner/core/plan_cache.result
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/retest |
1 similar comment
|
/retest |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/retest |
1 similar comment
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AilinKid, D3Hunter, hawkingrei, terry1purcell 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 |
2 similar comments
|
/retest |
|
/retest |
|
/test pull-unit-test-next-gen |
|
@qw4990: The specified target(s) for Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test pull-integration-e2e-test |
|
@qw4990: The specified target(s) for Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
5 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
… cache strategy (pingcap#67535) close pingcap#67409
5 participants
What problem does this PR solve?
Issue Number: close #67409
Problem Summary: planner: support use_plan_cache hint and binding under hint_only plan cache strategy
What changed and how does it work?
tidb_plan_cache_strategy:all(default): keep current behaviorhint_only: only cache plans whenuse_plan_cache()is presentuse_plan_cache().hint_onlybehavior for:use_plan_cache()coming from SQL binding is also honored.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