Codex/plan cache hint only#67412
Closed
qw4990 wants to merge 5 commits into
Closed
Conversation
ti-chi-bot
Bot
added
do-not-merge/invalid-title
do-not-merge/needs-linked-issue
do-not-merge/needs-tests-checked
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 |
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SessionVars
participant Parser
participant Planner
participant BindInfo
participant PlanCache
Client->>SessionVars: SET tidb_plan_cache_policy = 'hint_only'
Client->>Parser: PREPARE / EXECUTE stmt (contains or not use_plan_cache hint)
Parser->>Planner: AST + StmtHints (UsePlanCache)
Planner->>BindInfo: match SQL binding with cache
BindInfo-->>Planner: matchedBinding (may include use_plan_cache hint)
Planner->>Planner: allowPlanCacheByPolicy(session.Policy, stmtHints, matchedBinding)
alt policy allows cache
Planner->>PlanCache: New plan cache key (includes matchedBinding info)
PlanCache-->>Planner: cache hit / miss
else policy disallows cache
Planner-->>SessionVars: stmtCtx.WarnSkipPlanCache(...)
Planner->>Planner: generate plan without plan cache
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.11.4)Command failed 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 |
|
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/planner/core/casetest/plancache/plan_cache_suite_test.go (1)
1833-1839: Initialize@abefore the first execution block for deterministic intent.The first three executions currently depend on unset user-variable behavior. Set
@aup-front so the “hint_only but no hint => no cache hit” assertion is explicit.✅ Minimal tweak
tk.MustExec(fmt.Sprintf("prepare st from 'select * from %s where pk >= ?'", tableName)) +tk.MustExec("set `@a`=1") tk.MustExec("execute st using `@a`") tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("0"))As per coding guidelines, "Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/casetest/plancache/plan_cache_suite_test.go` around lines 1833 - 1839, The test relies on an unset user variable `@a` causing non-deterministic behavior; initialize `@a` before the first execute block to make the "hint_only but no hint => no cache hit" assertions deterministic: add a statement that sets `@a` (e.g., SET `@a` = <some value>) before the first prepare/execute sequence that uses the prepared statement "st" so all subsequent execute st using `@a` calls operate with a defined value and the @@last_plan_from_cache checks remain explicit.pkg/util/hint/hint.go (1)
417-418: Handle conflictingUSE_PLAN_CACHE()andIGNORE_PLAN_CACHE()explicitly.Right now both flags can be set to
truein one statement. Please add deterministic precedence (typicallyIGNORE_PLAN_CACHEwins) and emit a conflicting-hint warning.♻️ Suggested parser-side conflict handling
case HintIgnorePlanCache: + if stmtHints.UsePlanCache { + warns = append(warns, ErrWarnConflictingHint.FastGenByArgs("IGNORE_PLAN_CACHE()")) + } stmtHints.IgnorePlanCache = true case HintUsePlanCache: + if stmtHints.IgnorePlanCache { + warns = append(warns, ErrWarnConflictingHint.FastGenByArgs("USE_PLAN_CACHE()")) + break + } stmtHints.UsePlanCache = true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/hint/hint.go` around lines 417 - 418, When handling HintUsePlanCache in the switch (case HintUsePlanCache / stmtHints.UsePlanCache), ensure you detect and resolve a conflict with HintIgnorePlanCache: if stmtHints.IgnorePlanCache is already true, do not enable UsePlanCache (leave stmtHints.UsePlanCache false), otherwise set stmtHints.UsePlanCache = true; in the conflict path emit a deterministic conflicting-hint warning using the package’s statement-warning mechanism (e.g. call the existing stmtHints warning API or add a small helper like emitConflictingHintWarning) so IGNORE_PLAN_CACHE takes precedence and a warning is recorded.
🤖 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/planner/core/plan_cache_utils.go`:
- Line 95: The plan cache enabling logic currently calls
stmtCtx.EnablePlanCache() unconditionally for cacheable statements; modify that
call to enforce sessVars.PlanCachePolicy by only enabling the cache when either
sessVars.PlanCachePolicy != "hint_only" or when the extracted PlanCacheStmt has
UsePlanCacheHint == true. Locate the conditional around
stmtCtx.EnablePlanCache() in plan_cache.go (where PlanCacheStmt is available
after usePlanCacheHint/hasUsePlanCacheHint extraction) and change it to check
sessVars.PlanCachePolicy and stmt.UsePlanCacheHint before calling
stmtCtx.EnablePlanCache().
In `@pkg/sessionctx/variable/session.go`:
- Line 2461: Add tests to cover the tidb_plan_cache_policy session variable: add
unit tests that assert the default value equals vardef.DefTiDBPlanCachePolicy
(verify reading in a fresh session), add a GLOBAL-scope test that attempts SET
@@global.tidb_plan_cache_policy = 'hint_only' and then opens a new session to
confirm whether global changes take effect (and if SetGlobal handler is missing,
update the session variable registration to implement SetGlobal for
PlanCachePolicy or assert that GLOBAL SET is a no-op), and add SQL integration
tests that exercise both 'all' (default) and 'hint_only' behaviors (e.g.,
prepared statement + binding cases) to validate runtime behavior; reference
tidb_plan_cache_policy, PlanCachePolicy and vardef.DefTiDBPlanCachePolicy when
locating registration and tests to update.
---
Nitpick comments:
In `@pkg/planner/core/casetest/plancache/plan_cache_suite_test.go`:
- Around line 1833-1839: The test relies on an unset user variable `@a` causing
non-deterministic behavior; initialize `@a` before the first execute block to make
the "hint_only but no hint => no cache hit" assertions deterministic: add a
statement that sets `@a` (e.g., SET `@a` = <some value>) before the first
prepare/execute sequence that uses the prepared statement "st" so all subsequent
execute st using `@a` calls operate with a defined value and the
@@last_plan_from_cache checks remain explicit.
In `@pkg/util/hint/hint.go`:
- Around line 417-418: When handling HintUsePlanCache in the switch (case
HintUsePlanCache / stmtHints.UsePlanCache), ensure you detect and resolve a
conflict with HintIgnorePlanCache: if stmtHints.IgnorePlanCache is already true,
do not enable UsePlanCache (leave stmtHints.UsePlanCache false), otherwise set
stmtHints.UsePlanCache = true; in the conflict path emit a deterministic
conflicting-hint warning using the package’s statement-warning mechanism (e.g.
call the existing stmtHints warning API or add a small helper like
emitConflictingHintWarning) so IGNORE_PLAN_CACHE takes precedence and a warning
is recorded.
🪄 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: e4b8c939-82e2-466d-88da-8866ee36ca7e
📒 Files selected for processing (9)
pkg/parser/ast/misc.gopkg/planner/core/casetest/plancache/BUILD.bazelpkg/planner/core/casetest/plancache/plan_cache_suite_test.gopkg/planner/core/plan_cache_utils.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/setvar_affect.gopkg/sessionctx/variable/sysvar.gopkg/util/hint/hint.go
| RegardNULLAsPoint: vardef.DefTiDBRegardNULLAsPoint, | ||
| AllowProjectionPushDown: vardef.DefOptEnableProjectionPushDown, | ||
| SkipMissingPartitionStats: vardef.DefTiDBSkipMissingPartitionStats, | ||
| PlanCachePolicy: vardef.DefTiDBPlanCachePolicy, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: find tests and related coverage for tidb_plan_cache_policy.
set -euo pipefail
echo "== References in implementation =="
rg -n -C2 'tidb_plan_cache_policy|PlanCachePolicy|DefTiDBPlanCachePolicy|hint_only|USE_PLAN_CACHE'
echo
echo "== References in Go tests =="
rg -n -C2 'tidb_plan_cache_policy|PlanCachePolicy|hint_only|USE_PLAN_CACHE' --iglob '*_test.go'
echo
echo "== References in SQL/integration test assets =="
rg -n -C2 'tidb_plan_cache_policy|hint_only|USE_PLAN_CACHE' --iglob '*.sql' --iglob '*.result' --iglob '*.test'Repository: pingcap/tidb
Length of output: 9835
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Test function containing tidb_plan_cache_policy =="
sed -n '1810,1860p' pkg/planner/core/casetest/plancache/plan_cache_suite_test.go
echo
echo "== Looking for test function name around line 1820 =="
sed -n '1800,1830p' pkg/planner/core/casetest/plancache/plan_cache_suite_test.go | head -40
echo
echo "== Search for other test files that might test plan cache policy =="
fd -e go -e test -e sql -e result -e test | xargs grep -l 'tidb_plan_cache_policy' 2>/dev/null | head -20
echo
echo "== Check for default value test scenarios =="
rg -n 'DefTiDBPlanCachePolicy|PlanCachePolicyAll' --type go pkg/sessionctx/ | grep -i test
echo
echo "== Check if GLOBAL scope is tested for this variable =="
rg -n 'global.*tidb_plan_cache_policy|tidb_plan_cache_policy.*global' --type go -iRepository: pingcap/tidb
Length of output: 4191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== All test functions in plan_cache_suite_test.go =="
rg '^func Test' pkg/planner/core/casetest/plancache/plan_cache_suite_test.go | head -30
echo
echo "== Search for SQL/integration test files in plancache directory =="
fd -e sql -e result -e test . pkg/planner/core/casetest/plancache/ 2>/dev/null | head -20
echo
echo "== Check for any default value behavior test =="
rg -n "PlanCachePolicyAll|DefTiDBPlanCachePolicy" --type go pkg/planner/core/casetest/ -A 2 -B 2
echo
echo "== Check GLOBAL variable scope test in sysvar.go =="
sed -n '1580,1610p' pkg/sessionctx/variable/sysvar.go
echo
echo "== List all test files in plancache directory =="
ls -la pkg/planner/core/casetest/plancache/*.go | head -20Repository: pingcap/tidb
Length of output: 1783
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check the sysvar.go definition again for scope =="
sed -n '1585,1600p' pkg/sessionctx/variable/sysvar.go
echo
echo "== Search for tests that set DEFAULT value or test 'all' policy =="
rg -n "PlanCachePolicyAll|tidb_plan_cache_policy.*all" --type go -i pkg/planner/ | head -20
echo
echo "== Search for tests that use GLOBAL scope (@@global. prefix) =="
rg -n "@@global.*tidb_plan_cache_policy|@@global.*PlanCachePolicy" --type go -i
echo
echo "== Check what the default value actually is =="
rg -n "DefTiDBPlanCachePolicy.*=" pkg/sessionctx/vardef/tidb_vars.go
echo
echo "== Look for any test that doesn't set the variable (implicit default test) =="
rg -n "func Test" pkg/planner/core/casetest/plancache/plan_cache_suite_test.go -A 15 | grep -A 15 "TestInitLRUWithSystemVar"
echo
echo "== Search integration tests directory structure =="
find pkg -type d -name "*test*" | grep -i integ | head -10
find pkg -type f -name "*.test" | head -10
find pkg -type f -name "*.sql" | head -10Repository: pingcap/tidb
Length of output: 1021
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check for SetGlobal function in sysvar.go for tidb_plan_cache_policy =="
sed -n '1585,1610p' pkg/sessionctx/variable/sysvar.go | cat -n
echo
echo "== Search for how other variables define SetGlobal =="
rg -n "SetGlobal:" pkg/sessionctx/variable/sysvar.go | head -5
echo
echo "== Check if tidb_plan_cache_policy has SetGlobal defined =="
sed -n '1585,1600p' pkg/sessionctx/variable/sysvar.go | grep -i setglobal || echo "No SetGlobal found"
echo
echo "== Search for tests that verify default behavior at session startup =="
rg -n "NewSessionVars|NewSessionWithOpts" pkg/planner/core/casetest/plancache/plan_cache_suite_test.go | head -5
echo
echo "== Look at beginning of plan_cache_suite_test.go to see imports and setup =="
head -60 pkg/planner/core/casetest/plancache/plan_cache_suite_test.go
echo
echo "== Check if there are dedicated integration test suites =="
fd -type f -name '*.test' | head -20Repository: pingcap/tidb
Length of output: 2649
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check for default SetGlobal behavior when SetGlobal is not defined =="
rg -n "SetGlobal.*nil\|if.*SetGlobal.*==\|SetGlobal.*==" pkg/sessionctx/variable/sysvar.go | head -10
echo
echo "== Look for how SetGlobal is used/handled in the system =="
rg -n "s.SetGlobal" pkg/sessionctx/variable/ | head -10
echo
echo "== Check if SetSession alone works for both GLOBAL and SESSION =="
rg -n "SetSession:" pkg/sessionctx/variable/sysvar.go | head -5
echo
echo "== Check how other GLOBAL|SESSION scope variables handle this =="
sed -n '1400,1450p' pkg/sessionctx/variable/sysvar.go | grep -A 10 "ScopeGlobal.*ScopeSession"
echo
echo "== Find the actual test of hint_only to understand what behavior is verified =="
sed -n '1800,1900p' pkg/planner/core/casetest/plancache/plan_cache_suite_test.goRepository: pingcap/tidb
Length of output: 128
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for any test that reads the default value without setting it =="
rg -n "tidb_plan_cache_policy" pkg/planner/core/casetest/plancache/plan_cache_suite_test.go -B 5 | head -40
echo
echo "== Check what TestInitLRUWithSystemVar does =="
sed -n '100,200p' pkg/planner/core/casetest/plancache/plan_cache_suite_test.go | grep -A 100 "func TestInitLRUWithSystemVar"
echo
echo "== Count how many times the variable is explicitly tested =="
rg -c "tidb_plan_cache_policy" pkg/planner/core/casetest/plancache/plan_cache_suite_test.go
echo
echo "== Check if setvar_affect.go (which flags variable for invalidation) has tests =="
fd -name '*setvar*test*' pkg/sessionctx/variable/
echo
echo "== Search for sql test files for session variables =="
fd -path '*/sessionctx/*' -name '*.test' -o -name '*.sql' 2>/dev/null | head -10Repository: pingcap/tidb
Length of output: 852
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check if variable supports GLOBAL SET by looking at SetGlobal in sysvar.go for this variable =="
sed -n '1585,1595p' pkg/sessionctx/variable/sysvar.go
echo
echo "== Search for any test that uses @@global.tidb_plan_cache_policy =="
rg '@@global.*tidb_plan_cache_policy' --type go
echo
echo "== Check integration/sql test files that might test plan cache =="
find . -path './tests' -prune -o -name '*.test' -type f -print 2>/dev/null | xargs grep -l 'tidb_plan_cache_policy' 2>/dev/null || echo "No .test files found with the variable"
echo
echo "== Check if there's an integration test suite for sessionctx =="
find . -path '*tests*' -name '*session*.test' -o -name '*variable*.test' 2>/dev/null | head -10
echo
echo "== Look at what the actual default value constant is =="
rg -n "DefTiDBPlanCachePolicy" pkg/sessionctx/vardef/tidb_vars.go -A 1 -B 1
echo
echo "== Check if there's documentation for test coverage patterns =="
cat AGENTS.md 2>/dev/null | grep -A 5 "session.*variable" || echo "No AGENTS.md guidance found"Repository: pingcap/tidb
Length of output: 995
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check how GLOBAL scope variables work without explicit SetGlobal =="
rg -n "func.*applySetGlobal\|SetGlobal.*nil\|SetGlobal.*func" pkg/sessionctx/variable/sysvar.go | head -10
echo
echo "== Check if ScopeGlobal without SetGlobal gets default behavior =="
grep -B 5 -A 5 "ScopeGlobal.*SetSession.*func" pkg/sessionctx/variable/sysvar.go | head -30
echo
echo "== Look for how variables with only SetSession handle global scope =="
rg -n "Scope:.*Global.*Session.*SetSession:" pkg/sessionctx/variable/sysvar.go -A 5 | head -50
echo
echo "== Verify the default value constant =="
grep -n "PlanCachePolicyAll\|DefTiDBPlanCachePolicy" pkg/sessionctx/vardef/tidb_vars.go | head -5
echo
echo "== Check if there are any other tests besides the one we found =="
rg -n "\.PlanCachePolicy\|tidb_plan_cache_policy" pkg/planner/core/casetest/plancache/plan_cache_suite_test.goRepository: pingcap/tidb
Length of output: 126
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== List all occurrences of tidb_plan_cache_policy in the entire codebase =="
rg 'tidb_plan_cache_policy' --type go --type sql -i
echo
echo "== Check the sysvar definition structure more carefully =="
sed -n '1580,1600p' pkg/sessionctx/variable/sysvar.go | head -30
echo
echo "== Look for how SetGlobal falls back when not defined =="
grep -r "SetGlobal == nil\|SetGlobal.*nil\|if.*SetGlobal" pkg/sessionctx/variable/ | head -10
echo
echo "== Check if variable is mentioned in testing docs or test helpers =="
find . -name "*test*helper*" -o -name "*test*util*" | xargs grep -l "session.*variable" 2>/dev/null | head -5Repository: pingcap/tidb
Length of output: 2581
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check how SetGlobal is used in variable.go to understand the behavior =="
grep -B 5 -A 10 "if sv.SetGlobal != nil" pkg/sessionctx/variable/variable.go | head -30
echo
echo "== Check if GLOBAL scope for enum variables requires SetGlobal =="
rg -n "ScopeGlobal.*TypeEnum" pkg/sessionctx/variable/sysvar.go -A 5 | head -50
echo
echo "== Look for any integration test that might test this variable at GLOBAL scope =="
find . -name "*.test" -type f 2>/dev/null | xargs grep -l "set global tidb_plan_cache_policy" 2>/dev/null | head -5 || echo "No .test files found"
echo
echo "== Check documentation or comments about the scope limitation =="
sed -n '1645,1650p' pkg/sessionctx/variable/session.goRepository: pingcap/tidb
Length of output: 6005
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check what happens when SetGlobal is nil and variable has ScopeGlobal =="
sed -n '1,100p' pkg/sessionctx/variable/variable.go | grep -A 30 "SetGlobalFromHook"
echo
echo "== Verify the complete SetGlobalFromHook logic =="
sed -n '230,270p' pkg/sessionctx/variable/variable.go
echo
echo "== Check if there's a test that tries to set GLOBAL scope =="
rg -n "set.*global.*tidb_plan_cache_policy" --type go -i
echo
echo "== Check setvar_affect.go to understand why this var is listed there =="
sed -n '110,125p' pkg/sessionctx/variable/setvar_affect.go
echo
echo "== Check if the variable has Aliases =="
rg -n "TiDBPlanCachePolicy.*Aliases\|Aliases.*PlanCachePolicy" pkg/sessionctx/variable/sysvar.goRepository: pingcap/tidb
Length of output: 131
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Read variable.go SetGlobalFromHook function =="
sed -n '85,125p' pkg/sessionctx/variable/variable.go
echo
echo "== Check if SetGlobal being nil is handled =="
sed -n '85,150p' pkg/sessionctx/variable/variable.go
echo
echo "== Check the complete definition of SysVar struct =="
sed -n '1,150p' pkg/sessionctx/variable/sysvar_definition.go | head -80Repository: pingcap/tidb
Length of output: 4660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find SetGlobalFromHook implementation =="
rg -n "func.*SetGlobalFromHook" pkg/sessionctx/variable/ -A 20
echo
echo "== Check if SetGlobal being nil causes issues or is handled =="
rg -n "sv.SetGlobal" pkg/sessionctx/variable/variable.go -B 3 -A 3
echo
echo "== Look at how the variable is initialized in session =="
sed -n '2450,2470p' pkg/sessionctx/variable/session.goRepository: pingcap/tidb
Length of output: 3942
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Verify what happens when GLOBAL SET is called on this variable =="
rg -n "SetGlobalSysVarOnly" pkg/sessionctx/variable/ | head -10
echo
echo "== Check if there are any tests that verify default value 'all' =="
rg -n "PlanCachePolicyAll\|'all'" pkg/planner/core/casetest/plancache/plan_cache_suite_test.go
echo
echo "== Check if any test tries to set GLOBAL scope =="
rg -n "@@global" pkg/planner/core/casetest/plancache/plan_cache_suite_test.go
echo
echo "== Verify if DefTiDBPlanCachePolicy equals PlanCachePolicyAll =="
rg -n "DefTiDBPlanCachePolicy\|PlanCachePolicyAll" pkg/sessionctx/vardef/tidb_vars.go | grep -A 1 -B 1 "1675\|2128"Repository: pingcap/tidb
Length of output: 1671
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Get line 1675 from tidb_vars.go to confirm default value =="
sed -n '1673,1677p' pkg/sessionctx/vardef/tidb_vars.go
echo
echo "== Get line 2128 from tidb_vars.go to confirm policy values =="
sed -n '2125,2132p' pkg/sessionctx/vardef/tidb_vars.go
echo
echo "== Count test occurrences in plan_cache_suite_test.go =="
wc -l pkg/planner/core/casetest/plancache/plan_cache_suite_test.go
echo
echo "== Verify TestPreparedPlanCacheHintOnlyWithBinding is the ONLY test using the variable =="
rg -n "tidb_plan_cache_policy\|PlanCachePolicy" pkg/planner/core/casetest/plancache/plan_cache_suite_test.go | wc -l
echo
echo "== Check if there's a test that doesn't explicitly set the variable (uses default) =="
rg -n "TestNonPreparedPlanCacheBasically\|TestInitLRUWithSystemVar" pkg/planner/core/casetest/plancache/plan_cache_suite_test.go -A 30 | head -50Repository: pingcap/tidb
Length of output: 1120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check if there are any .test files specifically for session variables =="
find . -path './tests' -name '*.test' -type f 2>/dev/null | xargs grep -l "tidb_plan_cache_policy" 2>/dev/null | head -5 || echo "No .test files found"
echo
echo "== Look for SQL integration test pattern in the repo =="
fd '.test' --max-depth 3 ./tests 2>/dev/null | head -10
echo
echo "== Confirm that TestPreparedPlanCacheHintOnlyWithBinding exists and what it covers =="
sed -n '1813,1900p' pkg/planner/core/casetest/plancache/plan_cache_suite_test.goRepository: pingcap/tidb
Length of output: 5140
Confirm test coverage for tidb_plan_cache_policy session variable — GLOBAL scope and default value tests are missing.
Test coverage exists for SESSION scope (TestPreparedPlanCacheHintOnlyWithBinding sets and verifies 'hint_only' behavior with bindings), but verification gaps remain:
- GLOBAL scope: No test for
SET @@global.tidb_plan_cache_policy(variable is defined withScopeGlobal|ScopeSessionbut lacks aSetGlobalhandler, which may cause GLOBAL SET to silently succeed without effect). - Default value behavior: No explicit test for the default
'all'policy. - SQL integration tests: None found.
Per guidelines for session variables, add targeted package tests for default value and GLOBAL scope, plus SQL integration tests for user-visible behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/sessionctx/variable/session.go` at line 2461, Add tests to cover the
tidb_plan_cache_policy session variable: add unit tests that assert the default
value equals vardef.DefTiDBPlanCachePolicy (verify reading in a fresh session),
add a GLOBAL-scope test that attempts SET @@global.tidb_plan_cache_policy =
'hint_only' and then opens a new session to confirm whether global changes take
effect (and if SetGlobal handler is missing, update the session variable
registration to implement SetGlobal for PlanCachePolicy or assert that GLOBAL
SET is a no-op), and add SQL integration tests that exercise both 'all'
(default) and 'hint_only' behaviors (e.g., prepared statement + binding cases)
to validate runtime behavior; reference tidb_plan_cache_policy, PlanCachePolicy
and vardef.DefTiDBPlanCachePolicy when locating registration and tests to
update.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67412 +/- ##
================================================
+ Coverage 77.7970% 78.0622% +0.2651%
================================================
Files 2023 2024 +1
Lines 556490 557669 +1179
================================================
+ Hits 432933 435329 +2396
+ Misses 121809 120584 -1225
- Partials 1748 1756 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. Notice: To remove the For example:
📖 For more info, you can check the "Contribute Code" section in the development guide. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/casetest/plancache/plan_cache_suite_test.go (1)
1891-1901: Consider explicitly dropping the first binding before creating the second.At line 1891, a second binding with
ignore_plan_cache()is created without first dropping theuse_plan_cache()binding from line 1880. While TiDB may implicitly replace bindings for the same pattern, explicitly dropping would make the test's intent clearer and avoid potential flakiness if binding replacement semantics change.Suggested change
+ tk.MustExec(fmt.Sprintf( + "DROP BINDING FOR select * from %s where pk >= ?", + tableName, + )) tk.MustExec(fmt.Sprintf( "CREATE BINDING FOR select * from %s where pk >= ? USING select /*+ ignore_plan_cache() */ * from %s where pk >= ?", tableName, tableName, ))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/casetest/plancache/plan_cache_suite_test.go` around lines 1891 - 1901, The test creates a second binding using ignore_plan_cache() for the same query pattern without explicitly removing the earlier use_plan_cache() binding, which can be flaky; before the line that creates the ignore_plan_cache() binding (the tk.MustExec that calls CREATE BINDING FOR select * from %s where pk >= ? USING select /*+ ignore_plan_cache() */ ...), add an explicit DROP BINDING FOR select * from %s where pk >= ? to remove the previous use_plan_cache() binding so the test intent is clear and deterministic.
🤖 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/casetest/plancache/plan_cache_suite_test.go`:
- Around line 1891-1901: The test creates a second binding using
ignore_plan_cache() for the same query pattern without explicitly removing the
earlier use_plan_cache() binding, which can be flaky; before the line that
creates the ignore_plan_cache() binding (the tk.MustExec that calls CREATE
BINDING FOR select * from %s where pk >= ? USING select /*+ ignore_plan_cache()
*/ ...), add an explicit DROP BINDING FOR select * from %s where pk >= ? to
remove the previous use_plan_cache() binding so the test intent is clear and
deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8796b381-9f5f-4c68-a410-a4663aaff019
📒 Files selected for processing (4)
pkg/planner/core/casetest/plancache/BUILD.bazelpkg/planner/core/casetest/plancache/plan_cache_suite_test.gopkg/planner/core/plan_cache.gopkg/planner/core/plan_cache_utils.go
✅ Files skipped from review due to trivial changes (1)
- pkg/planner/core/casetest/plancache/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/planner/core/plan_cache_utils.go
This was referenced Mar 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Behavior Changes
Bug Fixes
Tests