planner: recheck prepared preprocess sysvars#68194
Conversation
ti-chi-bot
Bot
added
the
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
size/L
📝 WalkthroughWalkthroughThis PR addresses plan cache preprocessing validation when system variables change during prepared statement execution. New fields track preprocessing configuration state (SQL mode, noop-funcs mode, shared-lock promotion) on cached statements. Logic differentiates schema-version failures from other preprocessing errors and re-checks preprocessing conditions before cache hits. A test validates behavior across variable enable/disable cycles. ChangesPlan Cache Preprocessing Checks for Sysvar Changes
Sequence DiagramsequenceDiagram
actor User
participant Session
participant PlanCache as Plan Cache<br/>(Planner)
participant Executor
rect rgba(200, 150, 255, 0.5)
Note over User,Executor: Prepare Statement (captures sysvar state)
User->>Session: PREPARE stmt FROM 'SELECT ... FOR SHARE'
Session->>PlanCache: GeneratePlanCacheStmtWithAST()
PlanCache->>PlanCache: Store PreprocessSQLMode,<br/>PreprocessNoopFuncsMode,<br/>PreprocessSharedLockPromotion
PlanCache-->>Session: Return PlanCacheStmt
Session-->>User: Prepared
end
rect rgba(255, 200, 150, 0.5)
Note over User,Executor: Change System Variable
User->>Session: SET @@tidb_enable_shared_lock_promotion = 0
Session->>Session: Update session sysvar
end
rect rgba(150, 200, 255, 0.5)
Note over User,Executor: Execute Statement (check if preprocess changed)
User->>Session: EXECUTE stmt
Session->>PlanCache: planCachePreprocess()
PlanCache->>PlanCache: Compare current session sysvars<br/>vs. stored PreprocessSQLMode, etc.
alt Preprocess vars changed
PlanCache->>PlanCache: Trigger re-preprocessing
PlanCache->>PlanCache: Validation fails<br/>(FOR SHARE not allowed)
PlanCache-->>Session: Return error
else Preprocess vars match
PlanCache-->>Session: Use cached plan
end
Session-->>User: Execution result/error
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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.
🧹 Nitpick comments (1)
pkg/executor/test/issuetest/executor_issue_test.go (1)
95-95: ⚡ Quick winUse an explicit non-Oracle
sql_modeinstead ofdefaultfor determinism.At Line 95,
set sql_mode = defaultcan become brittle if defaults change. Prefer setting an explicit non-Oracle mode (or empty mode) so this test remains stable.Proposed tweak
- tk.MustExec("set sql_mode = default") + tk.MustExec("set sql_mode = ''")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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/executor/test/issuetest/executor_issue_test.go` at line 95, The test currently runs tk.MustExec("set sql_mode = default") which is brittle; change this call so it sets an explicit, non-Oracle sql_mode string (for example an empty mode "" or a deterministic mode like "NO_ENGINE_SUBSTITUTION,STRICT_TRANS_TABLES") instead of "default". Locate the tk.MustExec invocation in executor_issue_test.go and replace the argument to set sql_mode with the chosen explicit mode so the test behavior is deterministic across environments.
🤖 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.
Nitpick comments:
In `@pkg/executor/test/issuetest/executor_issue_test.go`:
- Line 95: The test currently runs tk.MustExec("set sql_mode = default") which
is brittle; change this call so it sets an explicit, non-Oracle sql_mode string
(for example an empty mode "" or a deterministic mode like
"NO_ENGINE_SUBSTITUTION,STRICT_TRANS_TABLES") instead of "default". Locate the
tk.MustExec invocation in executor_issue_test.go and replace the argument to set
sql_mode with the chosen explicit mode so the test behavior is deterministic
across environments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6675167f-55b7-4af4-82a3-ea3439f78007
📒 Files selected for processing (5)
pkg/executor/test/issuetest/BUILD.bazelpkg/executor/test/issuetest/executor_issue_test.gopkg/planner/core/plan_cache.gopkg/planner/core/plan_cache_utils.gopkg/session/session.go
|
/retest |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68194 +/- ##
================================================
- Coverage 77.7620% 77.0571% -0.7050%
================================================
Files 1990 1972 -18
Lines 551774 554386 +2612
================================================
- Hits 429071 427194 -1877
- Misses 121783 127044 +5261
+ Partials 920 148 -772
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
1 participant
What problem does this PR solve?
Issue Number: close #65266
Problem Summary:
Prepared statements can skip
Preprocessvalidations after session variables that affect those checks change betweenPREPAREandEXECUTE. This can makeEXECUTEaccept SQL that direct execution would reject, such asFOR SHAREafter disabling shared-lock promotion or duplicate aliases after leaving Oracle SQL mode.What changed and how does it work?
This PR records the
Preprocess-sensitive session variables onPlanCacheStmtat prepare time:SQLModeDuring prepared execution,
planCachePreprocessnow rerunsPreprocesswhen those values differ from the current session state, even if the schema version is unchanged. Schema-change errors still keep the existingErrSchemaChangedwrapping, while sysvar-driven validation failures return the original validation error so they match direct execution behavior.The session-level prepare dedup cache also records the current
Preprocessstate when rebuilding a statement from the cached template.Check List
Tests
Focused validation reported for this patch:
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Release Notes
Tests
Bug Fixes