sessionctx/variable: add sysvar for new join reorder impl#66792
sessionctx/variable: add sysvar for new join reorder impl#66792guo-shaoge wants to merge 1 commit intopingcap:masterfrom
Conversation
Signed-off-by: guo-shaoge <shaoge1994@163.com>
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note
|
Review Complete Findings: 1 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
bot
added
the
size/XL
📝 WalkthroughWalkthroughA new system variable Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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
🤖 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/sessionctx/variable/setvar_affect.go`:
- Line 101: The sysvar "tidb_opt_enable_advanced_join_reorder" is listed as a
verified SET_VAR even though the planner does not read
SessionVars.TiDBOptEnableAdvancedJoinReorder; remove this entry from the
verified map in setvar_affect.go (or mark it unverified) so /*+ SET_VAR(...) */
does not falsely appear supported, or alternatively implement planner-side
consumption by having the planner read
SessionVars.TiDBOptEnableAdvancedJoinReorder before re-adding it to the verified
list.
In `@pkg/sessionctx/variable/sysvar.go`:
- Around line 2589-2592: The sysvar vardef.TiDBOptEnableAdvancedJoinReorder is
being registered and mutates SessionVars.TiDBOptEnableAdvancedJoinReorder but no
optimizer code consumes it; either remove/defer this registration or add the
consumer. Fix by either (A) removing the registration entry that references
vardef.TiDBOptEnableAdvancedJoinReorder and TiDBOptOn.set logic so the knob is
not user-visible, or (B) implement the consumer in the optimizer code paths to
read SessionVars.TiDBOptEnableAdvancedJoinReorder (where join reordering
decisions are made) and act on it; update tests to reflect the change. Ensure
changes reference the registration block that constructs the sysvar and the
SessionVars.TiDBOptEnableAdvancedJoinReorder field so reviewers can locate the
edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4ea40931-8805-447b-8dc4-2f2348186ebc
📒 Files selected for processing (7)
pkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/setvar_affect.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/varsutil_test.gotests/integrationtest/r/sessionctx/setvar.resulttests/integrationtest/t/sessionctx/setvar.test
| "tidb_opt_projection_push_down": {}, | ||
| "tidb_enable_vectorized_expression": {}, | ||
| "tidb_opt_join_reorder_threshold": {}, | ||
| "tidb_opt_enable_advanced_join_reorder": {}, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nC2 --type=go '\bTiDBOptEnableAdvancedJoinReorder\b|tidb_opt_enable_advanced_join_reorder' pkg testsRepository: pingcap/tidb
Length of output: 3763
Don't mark this SET_VAR as verified until the planner actually consumes it.
In the provided context, this knob is only wired into the session/sysvar layer (pkg/sessionctx/variable/session.go:1227-1228, 2349-2351 and pkg/sessionctx/variable/sysvar.go:2589-2591). A search of the codebase confirms there is no planner-side read of SessionVars.TiDBOptEnableAdvancedJoinReorder, so adding it to the verified list makes /*+ SET_VAR(...) */ look supported even though it won't affect join ordering yet.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/sessionctx/variable/setvar_affect.go` at line 101, The sysvar
"tidb_opt_enable_advanced_join_reorder" is listed as a verified SET_VAR even
though the planner does not read SessionVars.TiDBOptEnableAdvancedJoinReorder;
remove this entry from the verified map in setvar_affect.go (or mark it
unverified) so /*+ SET_VAR(...) */ does not falsely appear supported, or
alternatively implement planner-side consumption by having the planner read
SessionVars.TiDBOptEnableAdvancedJoinReorder before re-adding it to the verified
list.
| {Scope: vardef.ScopeGlobal | vardef.ScopeSession, Name: vardef.TiDBOptEnableAdvancedJoinReorder, Value: BoolToOnOff(vardef.DefTiDBOptEnableAdvancedJoinReorder), Type: vardef.TypeBool, SetSession: func(s *SessionVars, val string) error { | ||
| s.TiDBOptEnableAdvancedJoinReorder = TiDBOptOn(val) | ||
| return nil | ||
| }}, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the new sysvar is consumed outside variable plumbing/tests.
rg -n -C2 --type=go 'TiDBOptEnableAdvancedJoinReorder|tidb_opt_enable_advanced_join_reorder' \
-g '!tests/**' -g '!**/*_test.go'Repository: pingcap/tidb
Length of output: 3187
Don't register a user-visible knob without implementation.
This sysvar is exposed to users (global/session scope) and settable, but no optimizer code actually consumes SessionVars.TiDBOptEnableAdvancedJoinReorder yet. This creates a misleading no-op setting. Land the consumer code in this PR, or defer the sysvar registration until the feature is ready.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/sessionctx/variable/sysvar.go` around lines 2589 - 2592, The sysvar
vardef.TiDBOptEnableAdvancedJoinReorder is being registered and mutates
SessionVars.TiDBOptEnableAdvancedJoinReorder but no optimizer code consumes it;
either remove/defer this registration or add the consumer. Fix by either (A)
removing the registration entry that references
vardef.TiDBOptEnableAdvancedJoinReorder and TiDBOptOn.set logic so the knob is
not user-visible, or (B) implement the consumer in the optimizer code paths to
read SessionVars.TiDBOptEnableAdvancedJoinReorder (where join reordering
decisions are made) and act on it; update tests to reflect the change. Ensure
changes reference the registration block that constructs the sysvar and the
SessionVars.TiDBOptEnableAdvancedJoinReorder field so reviewers can locate the
edits.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #66792 +/- ##
================================================
- Coverage 77.6870% 77.5564% -0.1306%
================================================
Files 2007 1930 -77
Lines 549532 543497 -6035
================================================
- Hits 426915 421517 -5398
- Misses 120957 121956 +999
+ Partials 1660 24 -1636
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
| s.TiDBOptJoinReorderThreshold = tidbOptPositiveInt32(val, vardef.DefTiDBOptJoinReorderThreshold) | ||
| return nil | ||
| }}, | ||
| {Scope: vardef.ScopeGlobal | vardef.ScopeSession, Name: vardef.TiDBOptEnableAdvancedJoinReorder, Value: BoolToOnOff(vardef.DefTiDBOptEnableAdvancedJoinReorder), Type: vardef.TypeBool, SetSession: func(s *SessionVars, val string) error { |
There was a problem hiding this comment.
[P2] Sysvar tidb_opt_enable_advanced_join_reorder is added but never consulted by the planner
Why: The sysvar is settable/readable with a default of true, but no planner code path reads SessionVars.TiDBOptEnableAdvancedJoinReorder. The advanced join reorder (joinorder.Optimize) is only activated through the enableCDCJoinReorder failpoint, so in production the sysvar is a complete no-op. This creates user-visible confusion: a user can SET tidb_opt_enable_advanced_join_reorder=ON/OFF with no observable effect.
Evidence:
pkg/sessionctx/variable/sysvar.go:2589:SetSession: s.TiDBOptEnableAdvancedJoinReorder = TiDBOptOn(val)— field is written but never read back by the planner.pkg/planner/core/rule_join_reorder.go:260:failpoint.Inject("enableCDCJoinReorder" ...)— the only gate for the new framework is this failpoint, not the sysvar.pkg/sessionctx/vardef/tidb_vars.go:1528: Default istrue, making the no-op more misleading.
Suggested fix: Either wire the sysvar into the planner branch condition (replacing or complementing the failpoint), or keep the sysvar hidden/unexported until the new join reorder implementation is production-ready.
There was a problem hiding this comment.
will use this variable in next PR: #66349
There was a problem hiding this comment.
Thanks for the clarification! A staged approach makes sense. One small thing worth considering before #66349 lands: the current default is true (DefTiDBOptEnableAdvancedJoinReorder = true), which means users on this version will see the sysvar is "enabled" but it has no effect. Setting the default to false here and flipping it to true once the planner integration is complete in #66349 would avoid any potential confusion in the interim. Not a blocker — just something to keep in mind.
ℹ️ Learn more details on Pantheon AI.
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/retest |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: AilinKid, hawkingrei, qw4990 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 |
4 participants
What problem does this PR solve?
Issue Number: close #66793
Problem Summary: this PR add a new sys var for new join reorder impl, and it will be used in the next PR: #66349
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
tidb_opt_enable_advanced_join_reorderintroduced for query optimization control. Enabled by default. Can be configured globally, at session scope, or per-query using SET_VAR hints.