sessionctx/variable: add sysvar for new join reorder impl#66792
sessionctx/variable: add sysvar for new join reorder impl#66792ti-chi-bot[bot] merged 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.gh.lumenfield.work/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.
There was a problem hiding this comment.
will use this variable in next PR: #66349
There was a problem hiding this comment.
@guo-shaoge, thanks for the context! That makes sense as a staged approach. One thing to keep in mind: between the merge of this PR and the merge of #66349, there will be a window where /*+ SET_VAR(tidb_opt_enable_advanced_join_reorder=OFF) */ is silently a no-op — the hint will appear accepted (because it's in isHintUpdatableVerified) but won't actually affect join ordering yet.
If that window is expected to be short and both PRs are merged in close succession, it's a minor concern. However, if you'd like to be safer, consider moving the isHintUpdatableVerified entry to #66349 alongside the actual planner-side read of SessionVars.TiDBOptEnableAdvancedJoinReorder, so the two are introduced atomically. Up to you — I'll leave the decision to your judgment.
🧠 Learnings used
Learnt from: mjonss
Repo: pingcap/tidb PR: 66303
File: pkg/statistics/handle/storage/gc.go:170-172
Timestamp: 2026-03-05T16:38:28.156Z
Learning: In `pkg/statistics/handle/storage/gc.go` (pingcap/tidb), the `mysql.stats_table_data` table introduced in PR `#66303` does NOT store per-histogram (hist_id) granularity rows. Therefore, `deleteHistStatsFromKV` does NOT need to delete from `mysql.stats_table_data`. The table-level delete added in `DeleteTableStatsFromKV` is purely defensive programming for downgrade safety, since no data is currently written to the table.
| {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.
There was a problem hiding this comment.
will use this variable in next PR: #66349
There was a problem hiding this comment.
@guo-shaoge, understood — registering the sysvar ahead of the consumer PR is a reasonable approach. Since PR #66349 will introduce the actual optimizer code that reads SessionVars.TiDBOptEnableAdvancedJoinReorder, the knob will remain a no-op only in the interim between these two PRs.
Just a small note to keep in mind: once #66349 lands, please make sure the integration test in tests/integrationtest/ (already added here) actually exercises a query where the advanced join reorder path is taken/skipped, rather than just verifying the variable can be set/read — that will give stronger confidence in the end-to-end behavior.
🧠 Learnings used
Learnt from: mjonss
Repo: pingcap/tidb PR: 66303
File: pkg/statistics/handle/storage/gc.go:170-172
Timestamp: 2026-03-05T16:38:28.156Z
Learning: In `pkg/statistics/handle/storage/gc.go` (pingcap/tidb), the `mysql.stats_table_data` table introduced in PR `#66303` does NOT store per-histogram (hist_id) granularity rows. Therefore, `deleteHistStatsFromKV` does NOT need to delete from `mysql.stats_table_data`. The table-level delete added in `DeleteTableStatsFromKV` is purely defensive programming for downgrade safety, since no data is currently written to the table.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #66792 +/- ##
================================================
+ Coverage 77.6870% 77.8331% +0.1461%
================================================
Files 2007 1934 -73
Lines 549532 547196 -2336
================================================
- Hits 426915 425900 -1015
- Misses 120957 121273 +316
+ Partials 1660 23 -1637
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.gh.lumenfield.work/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 |
yudongusa
left a comment
yudongusa
left a comment
There was a problem hiding this comment.
Please open a document PR for this
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AilinKid, hawkingrei, qw4990, yudongusa The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
5 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.