pingcap / pingcap/tidb

planner: Missing initialization for EnableSemiJoinRewrite in NewSessionVars (release-8.5)

Open
#64,304 2 comments 0 reactions 0 assignees View on GitHub
sig/planner type/bug
Dominant language
Go
Stars
40.5k
Forks
6.2k
PR merge metrics
PR metrics pending

Description

## Bug Report

The \`EnableSemiJoinRewrite\` field in \`SessionVars\` struct is not initialized in the \`NewSessionVars()\` function, while it has a corresponding default constant \`DefOptEnableSemiJoinRewrite\` defined.

## Location

**File**: \`pkg/sessionctx/variable/session.go\`
**Branch**: \`release-8.5\`
**Function**: \`NewSessionVars()\`
**Affected Field**: \`EnableSemiJoinRewrite\`

## Current State

\`\`\`go
// Field declaration exists (line ~1115)
type SessionVars struct {
// EnableSemiJoinRewrite enables the SEMI_JOIN_REWRITE hint for subqueries in the where clause.
EnableSemiJoinRewrite bool
}

// Default constant exists (tidb_vars.go)
const DefOptEnableSemiJoinRewrite = false

// System variable registration exists (sysvar.go)
{Name: TiDBOptEnableSemiJoinRewrite, Value: BoolToOnOff(DefOptEnableSemiJoinRewrite), ...}

// ❌ BUT: Missing initialization in NewSessionVars()
func NewSessionVars(hctx HookContext) *SessionVars {
vars := &SessionVars{
EnableOuterJoinReorder: DefTiDBEnableOuterJoinReorder,
EnableNoDecorrelateInSelect: DefOptEnableNoDecorrelateInSelect,
// EnableSemiJoinRewrite: DefOptEnableSemiJoinRewrite, // ← MISSING!
RetryLimit: DefTiDBRetryLimit,
DisableTxnAutoRetry: DefTiDBDisableTxnAutoRetry,
...
}
}
\`\`\`

## Why This is a Bug

While the field currently works because:
1. Go's zero value for \`bool\` is \`false\`
2. \`DefOptEnableSemiJoinRewrite\` is currently \`false\`
3. So the zero value happens to match the expected default

This is still a bug because:

1. **Code Inconsistency**: All other optimizer variables with \`Def*\` constants are explicitly initialized
2. **Violates Code Convention**: The codebase pattern is to explicitly initialize all fields that have default constants
3. **Potential Future Bug**: If someone changes \`DefOptEnableSemiJoinRewrite\` to \`true\` in the future, new sessions will still get \`false\` (zero value), causing unexpected behavior
4. **Maintainability**: It's unclear whether the missing initialization is intentional or an oversight

## Impact

### Current Impact: Low
- Functionality works correctly because zero value matches default value
- No runtime errors or incorrect behavior observed

### Future Risk: High
If \`DefOptEnableSemiJoinRewrite\` is changed to \`true\`:
\`\`\`go
const DefOptEnableSemiJoinRewrite = true // Changed from false
\`\`\`

Then:
- **Global variable** would show: \`true\` ✅
- **New sessions** would get: \`false\` (uninitialized) ❌
- **Result**: Inconsistent behavior between global setting and new session default

## Proposed Fix

Add the missing initialization in \`NewSessionVars()\`:

\`\`\`diff
func NewSessionVars(hctx HookContext) *SessionVars {
vars := &SessionVars{
EnableOuterJoinReorder: DefTiDBEnableOuterJoinReorder,
EnableNoDecorrelateInSelect: DefOptEnableNoDecorrelateInSelect,
+ EnableSemiJoinRewrite: DefOptEnableSemiJoinRewrite,
RetryLimit: DefTiDBRetryLimit,
DisableTxnAutoRetry: DefTiDBDisableTxnAutoRetry,
...
}
}
\`\`\`

## Related PRs

- PR #63540: Added \`EnableSemiJoinRewrite\` field and its initialization
- PR #63541: Cherry-pick that accidentally removed the initialization

### 4. What is your TiDB version? (Required)

v8.5.4-pre

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.