planner, executor: include relevant opt vars in plan cache key#67836
planner, executor: include relevant opt vars in plan cache key#67836hawkingrei wants to merge 1 commit into
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
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 |
📝 WalkthroughWalkthroughThis PR resolves a plan cache bug where optimizer-relevant session variables like Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 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 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 |
![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 (1)
pkg/planner/core/plan_cache_utils.go (1)
879-881: Add doc comments on the new exportedPlanCacheStmtfields.Briefly document that these are the sorted set of optimizer vars/fix IDs that influenced planning for this statement and are hashed into the plan-cache key. As per coding guidelines: "Keep exported-symbol doc comments, and prefer semantic constraints over name restatement."
📝 Proposed doc
- RelevantOptVarNames []string - RelevantOptFixIDs []uint64 + // RelevantOptVarNames and RelevantOptFixIDs are the sorted, deterministic + // sets of optimizer variables / fix-control IDs that influenced this plan + // during GeneratePlanCacheStmtWithAST. They are read-only after creation + // and are hashed (together with their current session values) into the + // plan-cache key so that toggling any of them invalidates cached plans. + RelevantOptVarNames []string + RelevantOptFixIDs []uint64🤖 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 879 - 881, Add concise doc comments to the exported PlanCacheStmt fields RelevantOptVarNames and RelevantOptFixIDs explaining they are the sorted sets of optimizer variable names and optimizer fixed IDs that affected planning for this statement and that these values are included (hashed) in the plan-cache key; use semantic wording (e.g., "sorted set of optimizer variable names that influenced planning and are hashed into the plan-cache key") rather than restating field names, and place the comments immediately above the RelevantOptVarNames and RelevantOptFixIDs declarations in the PlanCacheStmt type.
🤖 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 413-417: The current pre-allocation for per-fix budget (hashLen +=
len(stmt.RelevantOptFixIDs) * 64) can under-estimate when OptimizerFixControl
values are longer than ~54 bytes; change the sizing to compute the exact
additional bytes by iterating the relevant fix IDs (stmt.RelevantOptFixIDs) and
summing 8 (codec.EncodeUint) + 1 + len(vars.OptimizerFixControl[fixID]) + 1 for
each fix value, adding that sum to hashLen (instead of the fixed 64 multiplier),
and/or replace the test-only panic/asserion around intest.InTest with a
non-fatal warning log if preferred; update references in the same function that
builds the cache key (uses stmt.RelevantOptVarNames, stmt.RelevantOptFixIDs,
vars.OptimizerFixControl, and hashLen).
- Around line 577-636: The switch in appendRelevantOptVarValue includes
vardef.TiDBOptAlwaysKeepJoinKey but that option is never registered via
RecordRelevantOptVar; either remove the TiDBOptAlwaysKeepJoinKey case from
appendRelevantOptVarValue (dead code) or add a matching
RecordRelevantOptVar(vardef.TiDBOptAlwaysKeepJoinKey) call where other optimizer
vars are registered (place it alongside the other RecordRelevantOptVar(...)
calls in the planner initialization/registration code) so the variable is
actually recorded and the case is used.
---
Nitpick comments:
In `@pkg/planner/core/plan_cache_utils.go`:
- Around line 879-881: Add concise doc comments to the exported PlanCacheStmt
fields RelevantOptVarNames and RelevantOptFixIDs explaining they are the sorted
sets of optimizer variable names and optimizer fixed IDs that affected planning
for this statement and that these values are included (hashed) in the plan-cache
key; use semantic wording (e.g., "sorted set of optimizer variable names that
influenced planning and are hashed into the plan-cache key") rather than
restating field names, and place the comments immediately above the
RelevantOptVarNames and RelevantOptFixIDs declarations in the PlanCacheStmt
type.
🪄 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: 367465da-abc7-4216-b8d9-b8da30240031
📒 Files selected for processing (2)
pkg/executor/test/plancache/plan_cache_test.gopkg/planner/core/plan_cache_utils.go
| if len(stmt.RelevantOptVarNames) > 0 || len(stmt.RelevantOptFixIDs) > 0 { | ||
| hashLen++ | ||
| hashLen += len(stmt.RelevantOptVarNames) * 128 | ||
| hashLen += len(stmt.RelevantOptFixIDs) * 64 | ||
| } |
There was a problem hiding this comment.
Per-fix hash budget may under-size the buffer for long OptimizerFixControl values.
Each fix contributes 8 (codec.EncodeUint) + 1 ('=') + len(fixVal) + 1 (';') bytes. fixVal is an arbitrary string from vars.OptimizerFixControl, so any user-set fix whose string value exceeds ~54 bytes will force a realloc and trip the intest.InTest assertion at Lines 526-530. Consider either computing the exact size (summing len(fixVal)s) or widening the per-fix allowance and/or softening the test-only panic to a warning.
🤖 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 413 - 417, The current
pre-allocation for per-fix budget (hashLen += len(stmt.RelevantOptFixIDs) * 64)
can under-estimate when OptimizerFixControl values are longer than ~54 bytes;
change the sizing to compute the exact additional bytes by iterating the
relevant fix IDs (stmt.RelevantOptFixIDs) and summing 8 (codec.EncodeUint) + 1 +
len(vars.OptimizerFixControl[fixID]) + 1 for each fix value, adding that sum to
hashLen (instead of the fixed 64 multiplier), and/or replace the test-only
panic/asserion around intest.InTest with a non-fatal warning log if preferred;
update references in the same function that builds the cache key (uses
stmt.RelevantOptVarNames, stmt.RelevantOptFixIDs, vars.OptimizerFixControl, and
hashLen).
| func appendRelevantOptVarValue(hash []byte, vars *variable.SessionVars, varName string) []byte { | ||
| switch varName { | ||
| case vardef.TiDBOptIndexScanCostFactor: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.IndexScanCostFactor)) | ||
| case vardef.TiDBOptIndexReaderCostFactor: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.IndexReaderCostFactor)) | ||
| case vardef.TiDBOptTableReaderCostFactor: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.TableReaderCostFactor)) | ||
| case vardef.TiDBOptTableFullScanCostFactor: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.TableFullScanCostFactor)) | ||
| case vardef.TiDBOptTableRangeScanCostFactor: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.TableRangeScanCostFactor)) | ||
| case vardef.TiDBOptTableRowIDScanCostFactor: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.TableRowIDScanCostFactor)) | ||
| case vardef.TiDBOptTableTiFlashScanCostFactor: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.TableTiFlashScanCostFactor)) | ||
| case vardef.TiDBOptIndexLookupCostFactor: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.IndexLookupCostFactor)) | ||
| case vardef.TiDBOptIndexMergeCostFactor: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.IndexMergeCostFactor)) | ||
| case vardef.TiDBOptSortCostFactor: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.SortCostFactor)) | ||
| case vardef.TiDBOptTopNCostFactor: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.TopNCostFactor)) | ||
| case vardef.TiDBOptLimitCostFactor: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.LimitCostFactor)) | ||
| case vardef.TiDBOptStreamAggCostFactor: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.StreamAggCostFactor)) | ||
| case vardef.TiDBOptHashAggCostFactor: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.HashAggCostFactor)) | ||
| case vardef.TiDBOptMergeJoinCostFactor: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.MergeJoinCostFactor)) | ||
| case vardef.TiDBOptHashJoinCostFactor: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.HashJoinCostFactor)) | ||
| case vardef.TiDBOptIndexJoinCostFactor: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.IndexJoinCostFactor)) | ||
| case vardef.TiDBOptOrderingIdxSelRatio: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.OptOrderingIdxSelRatio)) | ||
| case vardef.TiDBOptRiskEqSkewRatio: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.RiskEqSkewRatio)) | ||
| case vardef.TiDBOptRiskRangeSkewRatio: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.RiskRangeSkewRatio)) | ||
| case vardef.TiDBOptRiskGroupNDVSkewRatio: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.RiskGroupNDVSkewRatio)) | ||
| case vardef.TiDBOptCartesianJoinOrderThreshold: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.CartesianJoinOrderThreshold)) | ||
| case vardef.TiDBOptSelectivityFactor: | ||
| return codec.EncodeUint(hash, math.Float64bits(vars.SelectivityFactor)) | ||
| case vardef.TiDBOptPreferRangeScan: | ||
| return append(hash, bool2Byte(vars.GetAllowPreferRangeScan())) | ||
| case vardef.TiDBOptEnableNoDecorrelateInSelect: | ||
| return append(hash, bool2Byte(vars.EnableNoDecorrelateInSelect)) | ||
| case vardef.TiDBOptEnableSemiJoinRewrite: | ||
| return append(hash, bool2Byte(vars.EnableSemiJoinRewrite)) | ||
| case vardef.TiDBOptAlwaysKeepJoinKey: | ||
| return append(hash, bool2Byte(vars.AlwaysKeepJoinKey)) | ||
| default: | ||
| return hash | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Enumerate all RecordRelevantOptVar(...) call sites and the constant/name argument used.
rg -nP --type=go -C1 '\bRecordRelevantOptVar\s*\('
# Cross-reference with the case labels in appendRelevantOptVarValue
ast-grep --pattern $'func appendRelevantOptVarValue($_, $_, $_) []byte {
$$$
}'Repository: pingcap/tidb
Length of output: 16784
Remove the unused TiDBOptAlwaysKeepJoinKey case or add its corresponding RecordRelevantOptVar call.
The switch in appendRelevantOptVarValue handles TiDBOptAlwaysKeepJoinKey (line 631–632) but this variable is never recorded anywhere in the codebase via RecordRelevantOptVar(). Either remove the dead case or add the missing recording call in the planner where this variable is checked. All other recorded optimizer variables have matching cases in this function.
🤖 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 577 - 636, The switch in
appendRelevantOptVarValue includes vardef.TiDBOptAlwaysKeepJoinKey but that
option is never registered via RecordRelevantOptVar; either remove the
TiDBOptAlwaysKeepJoinKey case from appendRelevantOptVarValue (dead code) or add
a matching RecordRelevantOptVar(vardef.TiDBOptAlwaysKeepJoinKey) call where
other optimizer vars are registered (place it alongside the other
RecordRelevantOptVar(...) calls in the planner initialization/registration code)
so the variable is actually recorded and the case is used.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67836 +/- ##
================================================
- Coverage 77.5976% 77.1084% -0.4893%
================================================
Files 1982 1965 -17
Lines 548894 549006 +112
================================================
- Hits 425929 423330 -2599
- Misses 122160 125674 +3514
+ Partials 805 2 -803
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
|
@hawkingrei: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
1 participant
What problem does this PR solve?
Issue Number: close #64338
Problem Summary:
Prepared plan cache can incorrectly reuse a cached plan after
tidb_opt_enable_semi_join_rewriteflips between
ONandOFF. The optimizer already records this switch as relevant during planbuilding, but the prepared plan cache key did not preserve and hash relevant optimizer vars/fixes
across the prepared statement boundary.
What changed and how does it work?
PlanCacheStmtduringGeneratePlanCacheStmtWithAST()newPlanCacheKeyWithMatchedBinding()testPreparePlanCache4DifferentSystemVars()coveringtidb_opt_enable_semi_join_rewriteON/OFFflips with prepared plan cache enabled forsubqueries
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit