*: support cache prepared statement in plan cache#67820
Merged
Conversation
Signed-off-by: guo-shaoge <shaoge1994@163.com>
ti-chi-bot
Bot
added
the
release-note-none
|
@guo-shaoge 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
size/XL
|
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:
📝 WalkthroughWalkthroughAdded a session-level prepared-statement deduplication cache and plan-cache utilities to rebuild lightweight prepared statements from cached PlanCacheStmt templates by re-parsing and refreshing AST-dependent metadata when schema versions match. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Session
participant DedupCache as "Dedup Cache"
participant Parser
participant InfoSchema
participant Executor
Client->>Session: PrepareStmt(sql, charset, collation, db, sqlMode)
Session->>Session: Compute dedup key
Session->>DedupCache: GetPrepareStmtDedupCache(key)
alt Cache Hit
DedupCache-->>Session: PrepareStmtCacheEntry
Session->>InfoSchema: Check schema version
alt Schema Match
Session->>Parser: Re-parse SQL with session charset/collation
Parser-->>Session: stmtNode
Session->>Session: ExtractAndSortParamMarkers(stmtNode)
Session->>InfoSchema: CollectPlanCacheStmtInfo(stmtNode)
Session->>Session: Rebuild PlanCacheStmt from cached template + AST info
Session-->>Client: New stmtID, Fields, ParamCount
else Schema Mismatch
Session->>Executor: Full prepare flow
Executor-->>Session: PlanCacheStmt
Session->>DedupCache: SetPrepareStmtDedupCache(key, entry)
Session-->>Client: stmtID, Fields, ParamCount
end
else Cache Miss
Session->>Executor: Full prepare flow
Executor-->>Session: PlanCacheStmt
Session->>DedupCache: SetPrepareStmtDedupCache(key, entry)
Session-->>Client: stmtID, Fields, ParamCount
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🧹 Nitpick comments (2)
pkg/planner/core/plan_cache_utils.go (1)
794-800: MakeCollectPlanCacheStmtInfoidempotent before appending derived fieldsIf this helper is called more than once on the same
PlanCacheStmt,limits/tablesare appended repeatedly and stale entries remain. Clearing before walk avoids subtle reuse bugs.Proposed refactor
func CollectPlanCacheStmtInfo(ctx context.Context, is infoschema.InfoSchema, stmt *PlanCacheStmt, stmtNode ast.StmtNode) { + stmt.limits = stmt.limits[:0] + stmt.tables = stmt.tables[:0] + stmt.hasSubquery = false processor := &planCacheStmtProcessor{ctx: ctx, is: is, stmt: stmt} stmtNode.Accept(processor) }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_utils.go` around lines 794 - 800, CollectPlanCacheStmtInfo currently appends derived data to the given PlanCacheStmt causing duplicate/stale entries if called multiple times; before creating planCacheStmtProcessor and walking the AST, clear the stmt's derived fields (e.g., stmt.limits, stmt.tables and any other slice/set fields populated by the processor such as stmt.hasSubquery flag if it is accumulated) so the walk is idempotent; locate CollectPlanCacheStmtInfo and reset those fields on the provided *PlanCacheStmt prior to stmtNode.Accept(processor).pkg/sessionctx/variable/session.go (1)
2900-2917: Add SQL integration coverage for this session/protocol behaviorThese APIs change PREPARE behavior at session scope; please ensure SQL integration tests cover charset/collation/currentDB-sensitive reuse and miss paths, not only unit-level checks.
Based on learnings, "For session variables or protocol behavior changes, perform targeted package tests 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` around lines 2900 - 2917, Add SQL integration and targeted package tests to cover session-scoped PREPARE dedup cache behavior implemented by SessionVars.GetPrepareStmtDedupCache and SessionVars.SetPrepareStmtDedupCache (cache size governed by SessionPlanCacheSize). Write package tests exercising the lazy initialization and LRU eviction of prepareStmtDedupCache, then add SQL integration tests that verify user-visible behavior: prepared-statement reuse and cache misses across changes to charset, collation, and current database in the same session (ensure prepared statements are reused only when charset/collation/currentDB match and missed/invalidated otherwise). Include assertions for both reuse paths and miss/invalidation paths and vary SessionPlanCacheSize to exercise eviction behavior.
🤖 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`:
- Around line 811-814: SetDBNameAndTbls currently assigns the caller's slices
directly which can cause shared backing array aliasing; instead make defensive
copies: allocate new slices and copy elements into s.dbName and s.tbls so
mutations in planCachePreprocess (which changes stmt.tbls[i]) cannot affect
other statements. In other words, in PlanCacheStmt.SetDBNameAndTbls create
s.dbName as a copy of the incoming dbName (e.g. via append to a nil slice) and
allocate/copy each element of tbls into a new slice assigned to s.tbls. Ensure
this handles nil/empty inputs correctly.
In `@pkg/session/test/common/prepare_dedup_cache_test.go`:
- Line 1: Update the file header copyright year from 2025 to 2026 by changing
the top-of-file comment line that currently reads "Copyright 2025 PingCAP, Inc."
to "Copyright 2026 PingCAP, Inc."; locate this string in
prepare_dedup_cache_test.go and modify it accordingly.
---
Nitpick comments:
In `@pkg/planner/core/plan_cache_utils.go`:
- Around line 794-800: CollectPlanCacheStmtInfo currently appends derived data
to the given PlanCacheStmt causing duplicate/stale entries if called multiple
times; before creating planCacheStmtProcessor and walking the AST, clear the
stmt's derived fields (e.g., stmt.limits, stmt.tables and any other slice/set
fields populated by the processor such as stmt.hasSubquery flag if it is
accumulated) so the walk is idempotent; locate CollectPlanCacheStmtInfo and
reset those fields on the provided *PlanCacheStmt prior to
stmtNode.Accept(processor).
In `@pkg/sessionctx/variable/session.go`:
- Around line 2900-2917: Add SQL integration and targeted package tests to cover
session-scoped PREPARE dedup cache behavior implemented by
SessionVars.GetPrepareStmtDedupCache and SessionVars.SetPrepareStmtDedupCache
(cache size governed by SessionPlanCacheSize). Write package tests exercising
the lazy initialization and LRU eviction of prepareStmtDedupCache, then add SQL
integration tests that verify user-visible behavior: prepared-statement reuse
and cache misses across changes to charset, collation, and current database in
the same session (ensure prepared statements are reused only when
charset/collation/currentDB match and missed/invalidated otherwise). Include
assertions for both reuse paths and miss/invalidation paths and vary
SessionPlanCacheSize to exercise eviction behavior.
🪄 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: 8f11439b-1893-43a1-b6d6-7d741cca4572
📒 Files selected for processing (4)
pkg/planner/core/plan_cache_utils.gopkg/session/session.gopkg/session/test/common/prepare_dedup_cache_test.gopkg/sessionctx/variable/session.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67820 +/- ##
================================================
+ Coverage 77.8007% 78.8080% +1.0073%
================================================
Files 1983 1978 -5
Lines 549049 560227 +11178
================================================
+ Hits 427164 441504 +14340
+ Misses 120965 118275 -2690
+ Partials 920 448 -472
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Signed-off-by: guo-shaoge <shaoge1994@163.com>
Signed-off-by: guo-shaoge <shaoge1994@163.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pkg/session/session.go (1)
3186-3186: Minor: drop the== truecomparison.Idiomatic Go is
if s.sessionVars.EnableCachePrepareStmt { … }. Applies to both Line 3186 and Line 3223.Proposed tweak
- if s.sessionVars.EnableCachePrepareStmt == true { + if s.sessionVars.EnableCachePrepareStmt {(at both sites)
Also applies to: 3223-3223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/session/session.go` at line 3186, Remove the redundant boolean comparison and use idiomatic Go by changing the condition checks that compare s.sessionVars.EnableCachePrepareStmt to true into a direct truthy check; locate both occurrences that currently read something like if s.sessionVars.EnableCachePrepareStmt == true and replace them with if s.sessionVars.EnableCachePrepareStmt { … } (ensure both the check near the prepare-stmt handling and the second occurrence are updated).
🤖 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/session.go`:
- Around line 3198-3205: The code reserves a prepared-stmt ID via
s.sessionVars.GetNextPreparedStmtID() then returns that ID into the named return
stmtID even when s.sessionVars.AddPreparedStmt fails; fix by preventing the
leaked ID: either use a local variable for the reserved ID (e.g., newID :=
s.sessionVars.GetNextPreparedStmtID()) and only assign it to the named return
stmtID after AddPreparedStmt succeeds, or explicitly reset stmtID = 0
immediately on AddPreparedStmt error before returning. Update the block around
s.sessionVars.GetNextPreparedStmtID and s.sessionVars.AddPreparedStmt
accordingly (and keep the existing s.rollbackOnError(ctx) call).
- Around line 3222-3231: The current code uses unsafe type assertions for
prepareExec.Stmt and cached entries which can panic on concrete type changes;
update both the cache-hit path (where you read from the dedup cache) and the
store path here to use comma-ok type assertions when converting prepareExec.Stmt
to *plannercore.PlanCacheStmt (and when casting any cached value you retrieve),
and simply skip caching or cache retrieval if the assertion fails; locate usages
around prepareExec.Stmt and the sessionVars
SetPrepareStmtDedupCache/GetPrepareStmtDedupCache calls and add the comma-ok
checks so you only call SetPrepareStmtDedupCache with a confirmed
*plannercore.PlanCacheStmt.
- Around line 3184-3211: The dedup key built by PrepareDedupCacheKey (used
around s.sessionVars.GetPrepareStmtDedupCache and rebuildFromPrepareCache) omits
session settings (notably sql_mode and timezone) that affect AST parsing and
cacheability (IsASTCacheable/preprocess); update the dedup key to include
sql_mode and any other sessionVars that preprocess/IsASTCacheable consult (e.g.,
PIPES_AS_CONCAT, ORACLE flags, time_zone) or alternatively add explicit
invalidation hooks to the prepare-dedup cache when those sessionVars change;
modify PrepareDedupCacheKey (and places that call it) to accept and incorporate
s.sessionVars.GetSQLMode()/GetTimeZone()/relevant flags so cached
PrepareStmtCacheEntry entries are not erroneously reused across differing modes.
- Around line 3260-3279: rebuildFromPrepareCache is reusing
cached.Stmt.ResolveCtx which maps TableName pointers from the old AST, causing
resolveCtx.GetTableName(newTableNamePtr) to miss and panic during plan building
(e.g., buildInsert calling tnW.TableInfo). Fix by refreshing the ResolveCtx for
the new AST inside rebuildFromPrepareCache (re-run Preprocess or rebuild
ResolveCtx.tableNames against newStmt.PreparedAst.Stmt) so keys point to the new
AST nodes; alternatively, if you prefer the call-site mitigation, add nil-checks
after ResolveCtx.GetTableName calls (e.g., in buildInsert and other
planbuilder.go sites) and return nil-safe handling, but prefer rebuilding
ResolveCtx to keep behavior consistent with planCachePreprocess/Preprocess.
---
Nitpick comments:
In `@pkg/session/session.go`:
- Line 3186: Remove the redundant boolean comparison and use idiomatic Go by
changing the condition checks that compare s.sessionVars.EnableCachePrepareStmt
to true into a direct truthy check; locate both occurrences that currently read
something like if s.sessionVars.EnableCachePrepareStmt == true and replace them
with if s.sessionVars.EnableCachePrepareStmt { … } (ensure both the check near
the prepare-stmt handling and the second occurrence are updated).
🪄 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: 79d47e9d-2236-42a8-b215-21a8ac5c6091
📒 Files selected for processing (7)
pkg/session/session.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/setvar_affect.gopkg/sessionctx/variable/sysvar.gotests/integrationtest/r/sessionctx/setvar.resulttests/integrationtest/t/sessionctx/setvar.test
✅ Files skipped from review due to trivial changes (1)
- pkg/sessionctx/variable/sysvar.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/sessionctx/variable/session.go
Comment on lines
+3198
to
+3205
| stmtID = s.sessionVars.GetNextPreparedStmtID() | ||
| if err = s.sessionVars.AddPreparedStmt(stmtID, newStmt); err != nil { | ||
| return | ||
| } | ||
| paramCount = cached.ParamCount | ||
| fields = cached.Fields | ||
| s.rollbackOnError(ctx) | ||
| return |
There was a problem hiding this comment.
Named return stmtID leaks out if AddPreparedStmt fails.
stmtID = s.sessionVars.GetNextPreparedStmtID() reserves a new ID, then on AddPreparedStmt failure the function returns with both err != nil and a non-zero stmtID. The counter has been advanced but no statement is registered, and a caller that inspects stmtID before checking err could act on an invalid ID. The full-prepare path below doesn't have this issue because it returns prepareExec.ID only on success (line 3232).
Consider resetting on error:
Proposed fix
- stmtID = s.sessionVars.GetNextPreparedStmtID()
- if err = s.sessionVars.AddPreparedStmt(stmtID, newStmt); err != nil {
- return
- }
+ newID := s.sessionVars.GetNextPreparedStmtID()
+ if err = s.sessionVars.AddPreparedStmt(newID, newStmt); err != nil {
+ return
+ }
+ stmtID = newID📝 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.
Suggested change
| stmtID = s.sessionVars.GetNextPreparedStmtID() | |
| if err = s.sessionVars.AddPreparedStmt(stmtID, newStmt); err != nil { | |
| return | |
| } | |
| paramCount = cached.ParamCount | |
| fields = cached.Fields | |
| s.rollbackOnError(ctx) | |
| return | |
| newID := s.sessionVars.GetNextPreparedStmtID() | |
| if err = s.sessionVars.AddPreparedStmt(newID, newStmt); err != nil { | |
| return | |
| } | |
| stmtID = newID | |
| paramCount = cached.ParamCount | |
| fields = cached.Fields | |
| s.rollbackOnError(ctx) | |
| return |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/session/session.go` around lines 3198 - 3205, The code reserves a
prepared-stmt ID via s.sessionVars.GetNextPreparedStmtID() then returns that ID
into the named return stmtID even when s.sessionVars.AddPreparedStmt fails; fix
by preventing the leaked ID: either use a local variable for the reserved ID
(e.g., newID := s.sessionVars.GetNextPreparedStmtID()) and only assign it to the
named return stmtID after AddPreparedStmt succeeds, or explicitly reset stmtID =
0 immediately on AddPreparedStmt error before returning. Update the block around
s.sessionVars.GetNextPreparedStmtID and s.sessionVars.AddPreparedStmt
accordingly (and keep the existing s.rollbackOnError(ctx) call).
Collaborator
Author
There was a problem hiding this comment.
minor issue, ignore
There was a problem hiding this comment.
@guo-shaoge, understood — I'll defer to your judgment on this one.
(╯°□°）╯
🧠 Learnings used
Learnt from: guo-shaoge
Repo: pingcap/tidb PR: 67857
File: pkg/session/test/common/prepare_dedup_cache_test.go:29-52
Timestamp: 2026-04-17T14:08:27.332Z
Learning: In `pkg/sessionctx/variable/tidb_vars.go` (pingcap/tidb), `DefEnableCachePrepareStmt = true`, meaning the `tidb_enable_cache_prepare_stmt` session variable is **enabled by default**. Tests for the prepare-stmt dedup-cache feature do not need an explicit `SET `@tidb_enable_cache_prepare_stmt` = ON` — the cache is active in all test sessions by default.
Learnt from: guo-shaoge
Repo: pingcap/tidb PR: 67857
File: pkg/planner/core/plan_cache_utils.go:621-624
Timestamp: 2026-04-17T14:03:09.042Z
Learning: In `pkg/planner/core/plan_cache_utils.go` (pingcap/tidb), `CollectPlanCacheStmtInfo` is always called with a **newly constructed** `PlanCacheStmt` (created inside `rebuildFromPrepareCache` in `pkg/session/session.go`), not a reused one. The `limits`, `hasSubquery`, and `tables` fields are therefore always zero-valued on entry. Do not flag the absence of reset/clear guards in `CollectPlanCacheStmtInfo` as a potential stale-data bug.
Learnt from: hawkingrei
Repo: pingcap/tidb PR: 67499
File: pkg/executor/prepared_test.go:0-0
Timestamp: 2026-04-13T03:14:21.604Z
Learning: In `pkg/executor/prepared_test.go` (pingcap/tidb), `TestExecuteWithWrongType` verifies that prepared statements with `c2 IN (?, ?)` on a `decimal(32,30)` column remain executable even when a wrong-type parameter (e.g., string `'aa'`) is passed. Both the first and subsequent executions always report `@last_plan_from_cache = 0` because wrong-type parameter shapes (type mismatch vs. the cached plan's shape) cause the plan cache to be bypassed entirely for all executions of that prepared statement. Asserting `1` for the second execution is incorrect; the stable expected value is `0` throughout.
Learnt from: cryo-zd
Repo: pingcap/tidb PR: 65532
File: pkg/planner/core/preprocess.go:754-760
Timestamp: 2026-03-13T00:34:41.277Z
Learning: In TiDB's AST visitor pattern (pkg/planner/core/preprocess.go), when a child node's Accept returns (node, false), the parent node's Accept propagates false upward WITHOUT calling Leave on the parent. Because preprocessor.Leave returns `ok = (p.err == nil)`, any subtree error causes the entire walk to short-circuit before the enclosing SelectStmt's Leave is reached. Therefore, code inside `case *ast.SelectStmt:` in `Leave` (e.g., checkLockClauseTables) is only ever executed when all children completed successfully and p.err is still nil. A guard like `if p.err == nil { checkLockClauseTables(...) }` would be purely cosmetic/defensive, not fixing any reachable bug.
Learnt from: joechenrh
Repo: pingcap/tidb PR: 67054
File: pkg/executor/importer/kv_encode.go:89-96
Timestamp: 2026-03-18T01:55:56.861Z
Learning: In `pkg/executor/importer/kv_encode.go` (pingcap/tidb), `TableKVEncoder.initInsertColFileMapping` uses `intest.Assert` (not a returned error) to guard the invariant that `fieldMappings` column IDs match `insertColumns` in order. This invariant is established at construction time and is covered by the broader test suite; it cannot be violated in production. Suggesting a production `error` return for this check is a false positive — the `intest.Assert` pattern is intentional and preferred here.
Learnt from: winoros
Repo: pingcap/tidb PR: 67411
File: pkg/planner/core/plan_cache_utils.go:0-0
Timestamp: 2026-04-01T11:13:21.523Z
Learning: In pingcap/tidb PR `#67411` (`pkg/planner/core/rule/rule_collect_plan_stats.go`), the correct fix for sync-load timeout fallback plans being cached with stale/pseudo stats is to call `stmtCtx.SetSkipPlanCache(skipPlanCacheReasonSyncLoadFallback)` immediately when `RequestLoadStats` or `SyncWaitStatsLoad` fails under `vardef.StatsLoadPseudoTimeout`. This prevents the fallback plan from ever being written to the plan cache, avoiding the stale-entry re-hit problem. Do not suggest the alternative of caching the fallback plan and invalidating it on fresh stats (via `SyncLoadFallbackItems` or similar mechanisms), as that approach leaves a stale entry resident in the cache.
Learnt from: joechenrh
Repo: pingcap/tidb PR: 66878
File: pkg/planner/core/logical_plan_builder.go:6455-6472
Timestamp: 2026-03-11T06:29:00.122Z
Learning: Ensure code reviews verify the VirtualAssignmentsOffset semantics: the planner sets Update.VirtualAssignmentsOffset = len(update.List). The executor should only apply OrderedList[:VirtualAssignmentsOffset] when composing new rows, and only after the 'changed' check should it group/evaluate OrderedList[VirtualAssignmentsOffset:] per table. This pattern applies to files under pkg/planner/core and pkg/executor (e.g., common_plans.go and update.go). Reviewers should check that updates respect slicing behavior, that the offset is consistently derived from the planner, and that downstream code does not bypass the offset when creating new rows. Add tests validating both branches: the slice before the offset for new rows, and the per-table handling of the slice after the offset.
Comment on lines
+3222
to
+3231
| // Store the result in the dedup cache for future Prepares of the same SQL. | ||
| if s.sessionVars.EnableCachePrepareStmt == true { | ||
| if prepareExec.Stmt != nil { | ||
| s.sessionVars.SetPrepareStmtDedupCache(dedupKey, &plannercore.PrepareStmtCacheEntry{ | ||
| Stmt: prepareExec.Stmt.(*plannercore.PlanCacheStmt), | ||
| Fields: prepareExec.Fields, | ||
| ParamCount: prepareExec.ParamCount, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm bounding / eviction policy for the new dedup cache.
rg -nP --type=go -C5 'SetPrepareStmtDedupCache|GetPrepareStmtDedupCache|PrepareStmtDedupCache\b' -g '!**/*_test.go'Repository: pingcap/tidb
Length of output: 3750
Use comma-ok type assertions to guard both cache hit and store operations against concrete type changes.
Both the cache-hit path (line 3191-3192) and the store path (line 3225) use unsafe type assertions that would panic if prepareExec.Stmt or cached values have unexpected concrete types. Wrap these assertions with comma-ok checks and skip caching on type mismatch instead.
The memory bounding concern is already addressed: SetPrepareStmtDedupCache creates an LRU cache bounded by SessionPlanCacheSize with automatic eviction, as documented at line 2915.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/session/session.go` around lines 3222 - 3231, The current code uses
unsafe type assertions for prepareExec.Stmt and cached entries which can panic
on concrete type changes; update both the cache-hit path (where you read from
the dedup cache) and the store path here to use comma-ok type assertions when
converting prepareExec.Stmt to *plannercore.PlanCacheStmt (and when casting any
cached value you retrieve), and simply skip caching or cache retrieval if the
assertion fails; locate usages around prepareExec.Stmt and the sessionVars
SetPrepareStmtDedupCache/GetPrepareStmtDedupCache calls and add the comma-ok
checks so you only call SetPrepareStmtDedupCache with a confirmed
*plannercore.PlanCacheStmt.
Collaborator
Author
There was a problem hiding this comment.
minor issue, ignore
There was a problem hiding this comment.
@guo-shaoge, understood — I'll drop this suggestion.
🧠 Learnings used
Learnt from: guo-shaoge
Repo: pingcap/tidb PR: 67857
File: pkg/session/test/common/prepare_dedup_cache_test.go:29-52
Timestamp: 2026-04-17T14:08:27.332Z
Learning: In `pkg/sessionctx/variable/tidb_vars.go` (pingcap/tidb), `DefEnableCachePrepareStmt = true`, meaning the `tidb_enable_cache_prepare_stmt` session variable is **enabled by default**. Tests for the prepare-stmt dedup-cache feature do not need an explicit `SET `@tidb_enable_cache_prepare_stmt` = ON` — the cache is active in all test sessions by default.
Learnt from: guo-shaoge
Repo: pingcap/tidb PR: 67857
File: pkg/planner/core/plan_cache_utils.go:621-624
Timestamp: 2026-04-17T14:03:09.042Z
Learning: In `pkg/planner/core/plan_cache_utils.go` (pingcap/tidb), `CollectPlanCacheStmtInfo` is always called with a **newly constructed** `PlanCacheStmt` (created inside `rebuildFromPrepareCache` in `pkg/session/session.go`), not a reused one. The `limits`, `hasSubquery`, and `tables` fields are therefore always zero-valued on entry. Do not flag the absence of reset/clear guards in `CollectPlanCacheStmtInfo` as a potential stale-data bug.
Learnt from: hawkingrei
Repo: pingcap/tidb PR: 67499
File: pkg/executor/prepared_test.go:0-0
Timestamp: 2026-04-13T03:14:21.604Z
Learning: In `pkg/executor/prepared_test.go` (pingcap/tidb), `TestExecuteWithWrongType` verifies that prepared statements with `c2 IN (?, ?)` on a `decimal(32,30)` column remain executable even when a wrong-type parameter (e.g., string `'aa'`) is passed. Both the first and subsequent executions always report `@last_plan_from_cache = 0` because wrong-type parameter shapes (type mismatch vs. the cached plan's shape) cause the plan cache to be bypassed entirely for all executions of that prepared statement. Asserting `1` for the second execution is incorrect; the stable expected value is `0` throughout.
Learnt from: joechenrh
Repo: pingcap/tidb PR: 67054
File: pkg/executor/importer/kv_encode.go:89-96
Timestamp: 2026-03-18T01:55:56.861Z
Learning: In `pkg/executor/importer/kv_encode.go` (pingcap/tidb), `TableKVEncoder.initInsertColFileMapping` uses `intest.Assert` (not a returned error) to guard the invariant that `fieldMappings` column IDs match `insertColumns` in order. This invariant is established at construction time and is covered by the broader test suite; it cannot be violated in production. Suggesting a production `error` return for this check is a false positive — the `intest.Assert` pattern is intentional and preferred here.
Learnt from: cryo-zd
Repo: pingcap/tidb PR: 65532
File: pkg/planner/core/preprocess.go:754-760
Timestamp: 2026-03-13T00:34:41.277Z
Learning: In TiDB's AST visitor pattern (pkg/planner/core/preprocess.go), when a child node's Accept returns (node, false), the parent node's Accept propagates false upward WITHOUT calling Leave on the parent. Because preprocessor.Leave returns `ok = (p.err == nil)`, any subtree error causes the entire walk to short-circuit before the enclosing SelectStmt's Leave is reached. Therefore, code inside `case *ast.SelectStmt:` in `Leave` (e.g., checkLockClauseTables) is only ever executed when all children completed successfully and p.err is still nil. A guard like `if p.err == nil { checkLockClauseTables(...) }` would be purely cosmetic/defensive, not fixing any reachable bug.
Learnt from: winoros
Repo: pingcap/tidb PR: 67411
File: pkg/planner/core/plan_cache_utils.go:0-0
Timestamp: 2026-04-01T11:13:21.523Z
Learning: In pingcap/tidb PR `#67411` (`pkg/planner/core/rule/rule_collect_plan_stats.go`), the correct fix for sync-load timeout fallback plans being cached with stale/pseudo stats is to call `stmtCtx.SetSkipPlanCache(skipPlanCacheReasonSyncLoadFallback)` immediately when `RequestLoadStats` or `SyncWaitStatsLoad` fails under `vardef.StatsLoadPseudoTimeout`. This prevents the fallback plan from ever being written to the plan cache, avoiding the stale-entry re-hit problem. Do not suggest the alternative of caching the fallback plan and invalidating it on fresh stats (via `SyncLoadFallbackItems` or similar mechanisms), as that approach leaves a stale entry resident in the cache.
Learnt from: cryo-zd
Repo: pingcap/tidb PR: 65532
File: pkg/planner/core/preprocess.go:1887-1898
Timestamp: 2026-03-12T08:41:44.509Z
Learning: In pkg/planner/core/preprocess.go (pingcap/tidb PR `#65532`), the `checkLockClauseTables` function intentionally accepts `FOR UPDATE OF <base_table>` even when the FROM item was introduced with an alias (`matched.alias.L != "" && !matchedByAlias`). Rather than returning an error (strict MySQL behavior), TiDB emits a warning to guide users toward the alias form. This backward-compatibility fallback is a deliberate acceptance criterion for the change — do not flag it as a MySQL-incompatibility bug in future reviews.
Learnt from: guo-shaoge
Repo: pingcap/tidb PR: 67284
File: pkg/planner/core/joinorder/join_order.go:386-406
Timestamp: 2026-03-26T09:18:03.097Z
Learning: In `pkg/planner/core/joinorder/join_order.go` (pingcap/tidb), the DP join reorder `optimizeWithDetector` function has a sanity guard `nodeCount > 63` before allocating `bestPlan := make([]*Node, 1<<nodeCount)`. This guard is off-by-one: when `nodeCount == 63`, `1<<63` overflows a signed int on 64-bit Go causing a `makeslice` panic. The correct guard should be `nodeCount >= 63`. A follow-up to reduce the effective DP threshold below 63 is tracked in issue `#67325`.
Learnt from: ChangRui-Ryan
Repo: pingcap/tidb PR: 67145
File: pkg/planner/core/casetest/tici/tici_test.go:470-487
Timestamp: 2026-03-24T10:04:49.982Z
Learning: In `pkg/planner/core/casetest/tici/tici_test.go` (pingcap/tidb PR `#67145`), TiCI FTS tests (e.g., TestTiCINonCoveringUsesDoubleRead, TestTiCIJoinNoMPPPlan, TestTiCIWithTiFlashReplicaNoMPPNoTiFlashCop) use positive assertions for `mpp[tiflash]` and do NOT include negative assertions for `mpp[tiflash]` or `ExchangeSender`. This is intentional: TiCI is MPP-only by design, so all TiCI-side plans are expected to include `mpp[tiflash]`. Do not suggest adding `CheckNotContain("mpp[tiflash]")` or `CheckNotContain("ExchangeSender")` to these TiCI test cases.
Learnt from: joechenrh
Repo: pingcap/tidb PR: 64752
File: pkg/planner/core/planbuilder.go:3987-3991
Timestamp: 2026-03-12T02:22:08.966Z
Learning: In `pkg/planner/core/planbuilder.go` (pingcap/tidb), `resolveGeneratedColumns` uses `onDups != nil` (not `len(onDups) > 0`) as the intentional guard for all mutations to the `onDups` map. `nil` means the IMPORT INTO / LOAD DATA call path (these callers pass nil explicitly, and the map must not be mutated). Non-nil — even if the map is empty — means the INSERT path; in this case ON UPDATE NOW columns must be recorded in `onDups` so that dependent generated columns propagate to `igc.OnDuplicates` when a duplicate-key conflict arises. Suggesting `len(onDups) > 0` instead of `onDups != nil` is a false positive: the nil/non-nil contract was established by PR `#63146` ("fix missing nilness check for passed onDup map") as a follow-up to PR `#58401`.
Learnt from: guo-shaoge
Repo: pingcap/tidb PR: 67284
File: pkg/util/intset/fast_int_set.go:227-229
Timestamp: 2026-03-25T08:26:10.360Z
Learning: In `pkg/util/intset/fast_int_set.go` (pingcap/tidb), `FastIntSet.GetSmallUInt64()` intentionally returns an error whenever `s.large != nil`, even if all remaining elements are within `[0, 64)`. This strict guard is by design: callers are expected to only pass sets that were never promoted to the large path. Do not flag this as a false positive.
Learnt from: joechenrh
Repo: pingcap/tidb PR: 66878
File: pkg/planner/core/logical_plan_builder.go:6455-6472
Timestamp: 2026-03-11T06:29:00.122Z
Learning: Ensure code reviews verify the VirtualAssignmentsOffset semantics: the planner sets Update.VirtualAssignmentsOffset = len(update.List). The executor should only apply OrderedList[:VirtualAssignmentsOffset] when composing new rows, and only after the 'changed' check should it group/evaluate OrderedList[VirtualAssignmentsOffset:] per table. This pattern applies to files under pkg/planner/core and pkg/executor (e.g., common_plans.go and update.go). Reviewers should check that updates respect slicing behavior, that the offset is consistently derived from the planner, and that downstream code does not bypass the offset when creating new rows. Add tests validating both branches: the slice before the offset for new rows, and the per-table handling of the slice after the offset.
There was a problem hiding this comment.
♻️ Duplicate comments (4)
pkg/session/session.go (4)
3192-3193:⚠️ Potential issue | 🟡 MinorGuard cache type assertions to prevent panics on unexpected concrete types.
Line 3193 and Line 3226 use unchecked type assertions. If cache content or stmt concrete type changes, this path panics instead of falling back safely.
Proposed fix
- if v := s.sessionVars.GetPrepareStmtDedupCache(dedupKey); v != nil { - cached := v.(*plannercore.PrepareStmtCacheEntry) + if v := s.sessionVars.GetPrepareStmtDedupCache(dedupKey); v != nil { + cached, ok := v.(*plannercore.PrepareStmtCacheEntry) + if !ok { + // unexpected cache entry type; fall through to full prepare path + } else { is := sessiontxn.GetTxnManager(s).GetTxnInfoSchema() if cached.Stmt.SchemaVersion == is.SchemaMetaVersion() { ... } + } } ... - if prepareExec.Stmt != nil { + if planStmt, ok := prepareExec.Stmt.(*plannercore.PlanCacheStmt); ok { s.sessionVars.SetPrepareStmtDedupCache(dedupKey, &plannercore.PrepareStmtCacheEntry{ - Stmt: prepareExec.Stmt.(*plannercore.PlanCacheStmt), + Stmt: planStmt, Fields: prepareExec.Fields, ParamCount: prepareExec.ParamCount, }) }Also applies to: 3225-3227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/session/session.go` around lines 3192 - 3193, The unchecked type assertions on the value returned by s.sessionVars.GetPrepareStmtDedupCache and on the cached statement (expected *plannercore.PrepareStmtCacheEntry and the concrete stmt type) can panic if the stored concrete types change; update the code in the block around s.sessionVars.GetPrepareStmtDedupCache(dedupKey) to use the comma-ok form for both assertions (e.g., v, ok := ...; entry, ok := v.(*plannercore.PrepareStmtCacheEntry)) and similarly guard the cached statement’s concrete type assertion, and when an assertion fails, treat it as a cache miss (clear or ignore the entry and proceed with the safe fallback path) and optionally log a debug message for visibility.
3190-3192:⚠️ Potential issue | 🟠 MajorInclude SQL mode (and other parse-affecting session state) in the dedup key.
Line 3191 keys only by SQL/charset/collation/currentDB. A session can change
sql_mode(and potentially timezone-dependent behavior), then hit this dedup entry and reuse stale prepare metadata (StmtCacheable/UncacheableReason) from a different parse context.Proposed fix
- dedupKey = variable.PrepareDedupCacheKey(sql, charset, collation, s.sessionVars.CurrentDB) + dedupKey = variable.PrepareDedupCacheKey( + sql, + charset, + collation, + s.sessionVars.CurrentDB, + s.sessionVars.SQLMode, + s.sessionVars.Location(), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/session/session.go` around lines 3190 - 3192, The dedup cache key (constructed via variable.PrepareDedupCacheKey and used when calling s.sessionVars.GetPrepareStmtDedupCache) only includes SQL, charset, collation and CurrentDB, so changes to session parse-affecting state like sql_mode (or timezone/other flags) can cause reuse of stale prepare metadata; update the key construction to also include s.sessionVars.GetSQLMode (and any other parse-affecting session variables you identify) so PrepareDedupCacheKey receives sql_mode (and timezone if relevant) when forming dedupKey before lookup, ensuring stmt cacheability and UncacheableReason are correct for the current session context.
3268-3270:⚠️ Potential issue | 🔴 CriticalDo not reuse cached
ResolveCtxwith a freshly parsed AST.Line 3269 reuses
cached.Stmt.ResolveCtxwhilePreparedAst.Stmtis newly parsed.ResolveCtxis tied to prior AST node identities; pointer-keyed lookups can miss and trigger nil dereference paths during later plan building.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/session/session.go` around lines 3268 - 3270, The code should not reuse cached.Stmt.ResolveCtx for a freshly parsed AST (PreparedAst.Stmt); instead recreate or clear the resolve context so pointer-keyed node lookups match the new AST. Replace the assignment of ResolveCtx: cached.Stmt.ResolveCtx with logic that either (a) builds a new ResolveCtx rooted/initialized for PreparedAst.Stmt, or (b) sets ResolveCtx to nil so the later resolver will create a fresh context tied to PreparedAst.Stmt; update any callers expecting a non-nil ResolveCtx accordingly (refer to symbols ResolveCtx, cached.Stmt.ResolveCtx, and PreparedAst.Stmt).
3198-3201:⚠️ Potential issue | 🟡 MinorAvoid returning a reserved
stmtIDwhen registration fails.Line 3198 assigns
stmtIDbeforeAddPreparedStmt. IfAddPreparedStmtfails, the function returnserrwith a non-zerostmtIDthat was never registered.Proposed fix
- stmtID = s.sessionVars.GetNextPreparedStmtID() - if err = s.sessionVars.AddPreparedStmt(stmtID, newStmt); err != nil { + newID := s.sessionVars.GetNextPreparedStmtID() + if err = s.sessionVars.AddPreparedStmt(newID, newStmt); err != nil { return } + stmtID = newID
🧹 Nitpick comments (2)
pkg/planner/core/plan_cache_utils.go (1)
779-792: Consider reusingExtractAndSortParamMarkersinGeneratePlanCacheStmtWithASTto eliminate duplication.The sort-by-offset +
SetOrderloop (lines 128–134) and the prep-pathDatum.SetNull()/InExecute=falsereset (lines 171–177) inGeneratePlanCacheStmtWithASTnow duplicate what this helper does. Folding the prepared-statement branch ofGeneratePlanCacheStmtWithASTto callExtractAndSortParamMarkerswould keep both code paths in lockstep — important because the rebuild path insession.gorelies on behavioral parity with the original prepare flow.Also, the doc comment should make the precondition explicit: the unconditional
Datum.SetNull()+InExecute=falsereset is only correct for the prepared-statement rebuild path (values not yet known). If a future caller applies this to a non-prepared/parameterized AST (whereParameterizeASThas already bound values), it would silently erase them and later affectcanParamMarkerBePositionExpr/ position-expr handling (seepkg/planner/core/logical_plan_builder.golines 113–119, 2465–2475).Suggested doc tightening
-// ExtractAndSortParamMarkers extracts ParamMarkerExpr nodes from the AST, -// sorts them by position, assigns their order indices, and initialises each -// marker's Datum to NULL (matching the behaviour of GeneratePlanCacheStmtWithAST -// for prepared statements where the actual parameter values are not yet known). +// ExtractAndSortParamMarkers extracts ParamMarkerExpr nodes from the AST, +// sorts them by position, assigns their order indices, and initialises each +// marker's Datum to NULL with InExecute=false. This matches the prepared-statement +// branch of GeneratePlanCacheStmtWithAST and MUST NOT be called on an AST whose +// parameters were already bound via ParameterizeAST (non-prepared path), as it +// would clobber those bound values.🤖 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 779 - 792, The prepared-statement branch in GeneratePlanCacheStmtWithAST duplicates the sort-by-offset + SetOrder and the reset logic (Datum.SetNull() / InExecute = false) already implemented in ExtractAndSortParamMarkers; refactor GeneratePlanCacheStmtWithAST to call ExtractAndSortParamMarkers (using the same paramMarkerExtractor/driver.ParamMarkerExpr handling) for the prepared-statement path to eliminate duplication and ensure behavioral parity with the rebuild flow, and update the ExtractAndSortParamMarkers doc comment to explicitly state the precondition that it resets param marker Datum and InExecute only for prepared-statement rebuilds (i.e., callers must not call it on ASTs where ParameterizeAST has already bound values).pkg/session/test/common/prepare_dedup_cache_test.go (1)
29-210: Settidb_enable_cache_prepare_stmtexplicitly in tests to make coverage deterministic.These tests currently rely on session defaults to hit the dedup path. Please set the variable explicitly (e.g., right after
NewTestKit) so the test keeps exercising this feature even if defaults change.Proposed fix
tk := testkit.NewTestKit(t, store) +tk.MustExec("set @@tidb_enable_cache_prepare_stmt = 1")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/session/test/common/prepare_dedup_cache_test.go` around lines 29 - 210, Add an explicit session setting to enable the prepare-statement dedup cache right after testkit.NewTestKit in each test to make behavior deterministic: in TestPrepareStmtDedupCacheBasic, TestPrepareStmtDedupCacheExecute, TestPrepareStmtDedupCacheSchemaChange, TestPrepareStmtDedupCacheIsolatedByDB, and TestPrepareStmtDedupCachePrepareExecuteCloseLoop call tk.MustExec to set tidb_enable_cache_prepare_stmt=1 (use the session/global syntax you prefer) immediately after tk := testkit.NewTestKit(...) so each test reliably exercises the dedup cache path regardless of defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/session/session.go`:
- Around line 3192-3193: The unchecked type assertions on the value returned by
s.sessionVars.GetPrepareStmtDedupCache and on the cached statement (expected
*plannercore.PrepareStmtCacheEntry and the concrete stmt type) can panic if the
stored concrete types change; update the code in the block around
s.sessionVars.GetPrepareStmtDedupCache(dedupKey) to use the comma-ok form for
both assertions (e.g., v, ok := ...; entry, ok :=
v.(*plannercore.PrepareStmtCacheEntry)) and similarly guard the cached
statement’s concrete type assertion, and when an assertion fails, treat it as a
cache miss (clear or ignore the entry and proceed with the safe fallback path)
and optionally log a debug message for visibility.
- Around line 3190-3192: The dedup cache key (constructed via
variable.PrepareDedupCacheKey and used when calling
s.sessionVars.GetPrepareStmtDedupCache) only includes SQL, charset, collation
and CurrentDB, so changes to session parse-affecting state like sql_mode (or
timezone/other flags) can cause reuse of stale prepare metadata; update the key
construction to also include s.sessionVars.GetSQLMode (and any other
parse-affecting session variables you identify) so PrepareDedupCacheKey receives
sql_mode (and timezone if relevant) when forming dedupKey before lookup,
ensuring stmt cacheability and UncacheableReason are correct for the current
session context.
- Around line 3268-3270: The code should not reuse cached.Stmt.ResolveCtx for a
freshly parsed AST (PreparedAst.Stmt); instead recreate or clear the resolve
context so pointer-keyed node lookups match the new AST. Replace the assignment
of ResolveCtx: cached.Stmt.ResolveCtx with logic that either (a) builds a new
ResolveCtx rooted/initialized for PreparedAst.Stmt, or (b) sets ResolveCtx to
nil so the later resolver will create a fresh context tied to PreparedAst.Stmt;
update any callers expecting a non-nil ResolveCtx accordingly (refer to symbols
ResolveCtx, cached.Stmt.ResolveCtx, and PreparedAst.Stmt).
---
Nitpick comments:
In `@pkg/planner/core/plan_cache_utils.go`:
- Around line 779-792: The prepared-statement branch in
GeneratePlanCacheStmtWithAST duplicates the sort-by-offset + SetOrder and the
reset logic (Datum.SetNull() / InExecute = false) already implemented in
ExtractAndSortParamMarkers; refactor GeneratePlanCacheStmtWithAST to call
ExtractAndSortParamMarkers (using the same
paramMarkerExtractor/driver.ParamMarkerExpr handling) for the prepared-statement
path to eliminate duplication and ensure behavioral parity with the rebuild
flow, and update the ExtractAndSortParamMarkers doc comment to explicitly state
the precondition that it resets param marker Datum and InExecute only for
prepared-statement rebuilds (i.e., callers must not call it on ASTs where
ParameterizeAST has already bound values).
In `@pkg/session/test/common/prepare_dedup_cache_test.go`:
- Around line 29-210: Add an explicit session setting to enable the
prepare-statement dedup cache right after testkit.NewTestKit in each test to
make behavior deterministic: in TestPrepareStmtDedupCacheBasic,
TestPrepareStmtDedupCacheExecute, TestPrepareStmtDedupCacheSchemaChange,
TestPrepareStmtDedupCacheIsolatedByDB, and
TestPrepareStmtDedupCachePrepareExecuteCloseLoop call tk.MustExec to set
tidb_enable_cache_prepare_stmt=1 (use the session/global syntax you prefer)
immediately after tk := testkit.NewTestKit(...) so each test reliably exercises
the dedup cache path regardless of defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4ef170b4-5304-420c-8b3e-e71994d232f1
📒 Files selected for processing (3)
pkg/planner/core/plan_cache_utils.gopkg/session/session.gopkg/session/test/common/prepare_dedup_cache_test.go
Collaborator
Author
|
/retest |
2 similar comments
Collaborator
Author
|
/retest |
Collaborator
Author
|
/retest |
Signed-off-by: guo-shaoge <shaoge1994@163.com>
qw4990
reviewed
Apr 18, 2026
qw4990
approved these changes
Apr 18, 2026
ti-chi-bot
Bot
added
approved
lgtm
and removed
needs-1-more-lgtm
14 tasks
Signed-off-by: guo-shaoge <shaoge1994@163.com>
Collaborator
Author
|
/retest |
17 similar comments
Collaborator
Author
|
/retest |
Member
|
/retest |
Member
|
/retest |
Member
|
/retest |
Member
|
/retest |
Member
|
/retest |
Member
|
/retest |
Member
|
/retest |
Member
|
/retest |
Member
|
/retest |
Member
|
/retest |
Member
|
/retest |
Member
|
/retest |
Member
|
/retest |
Member
|
/retest |
Member
|
/retest |
Member
|
/retest |
Member
|
/test all |
Member
|
/retest |
4 similar comments
Member
|
/retest |
Member
|
/retest |
Member
|
/retest |
Member
|
/retest |
13 tasks
premal
pushed a commit
to premal/tidb
that referenced
this pull request
Apr 30, 2026
This was referenced May 7, 2026
Merged
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
5 participants
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 #67815
Problem Summary: CPU profiling showed
HandleStmtPrepareconsuming ~21% of total CPU in workloads using the prepare-per-request pattern (COM_STMT_PREPARE → COM_STMT_EXECUTE →COM_STMT_CLOSE per request with the same SQL). Each Prepare redundantly executed Parse + Preprocess + PlanBuilder.Build for identical SQL text within the same
session.
What changed and how does it work?
Add a session-level LRU cache that deduplicates repeated
COM_STMT_PREPAREcalls for the same SQL. On a cache hit, we skip the expensivePlanBuilder.Buildstep(~9.6% CPU) while still re-parsing the SQL (to get independent AST/ParamMarkerExpr nodes) and re-running Preprocess (to get a fresh
ResolveCtxaligned with thenew AST).
Why re-parse instead of cloning the AST?
ParamMarkerExprnodes are written with parameter values during Execute. Each prepared statement needs its own AST tree.Why still run Preprocess?
ResolveCtx.tableNamesis amap[*ast.TableName]*TableNameWkeyed by AST pointer. Reusing the cachedResolveCtxwith a freshly-parsed AST would causenil-dereference panics on plan-cache miss, because the old pointer keys don't match the new AST's nodes.
Cache key
sql + charset + collation + currentDB + sqlMode— covers all session variables that affect parsing semantics.(todo might be better to add
enableWindowFunction + enableStrictDoubleTypeCheck, check #67857 (comment))Check List
Tests
./bench_client -set && ./bech_client -d '10m' -c 512 -conn 512 -threads 16bench_client.go.txt
we can see QPS increase from 8K to 10K wich CPU are both full:
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Tests