Skip to content
Merged
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
170 changes: 133 additions & 37 deletions pkg/planner/optimize.go
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,95 @@ var planBuilderPool = sync.Pool{
},
}

type logicalPlanBuildCtx struct {
stmtCtxState stmtctx.LogicalPlanBuildState
plannerSelectBlockAsName *[]ast.HintTable
}

func saveLogicalPlanBuildCtx(sessVars *variable.SessionVars) logicalPlanBuildCtx {
return logicalPlanBuildCtx{
stmtCtxState: sessVars.StmtCtx.SaveLogicalPlanBuildState(),
plannerSelectBlockAsName: sessVars.PlannerSelectBlockAsName.Load(),
}
}

func restoreLogicalPlanBuildCtx(sessVars *variable.SessionVars, logicalPlanCtx logicalPlanBuildCtx) {
sessVars.StmtCtx.RestoreLogicalPlanBuildState(logicalPlanCtx.stmtCtxState)
sessVars.PlannerSelectBlockAsName.Store(logicalPlanCtx.plannerSelectBlockAsName)
}

func buildAndOptimizeLogicalPlanRound(
ctx context.Context,
sctx planctx.PlanContext,
node *resolve.NodeW,
is infoschema.InfoSchema,
hintProcessor *hint.QBHintHandler,
checked *bool,
needRestoreLogicalPlanCtx bool,
bestPlan *base.PhysicalPlan,
bestNames *types.NameSlice,
bestCost *float64,
bestLogicalPlanCtx *logicalPlanBuildCtx,
) (base.Plan, types.NameSlice, bool, error) {
builder := planBuilderPool.Get().(*core.PlanBuilder)
defer planBuilderPool.Put(builder.ResetForReuse())

builder.Init(sctx, is, hintProcessor)

// todo: you can customize each round's special builder (like semi join rewrite or not by signal)
p, err := buildLogicalPlan(ctx, sctx, node, builder)
if err != nil {
return nil, nil, false, err
}
names := p.OutputNames()

if !*checked {
// Keep privilege and lock checks fail-fast. These depend on visitInfo
// produced by the logical build, but not on the later cost winner.
if pm := privilege.GetPrivilegeManager(sctx); pm != nil {
visitInfo := core.VisitInfo4PrivCheck(ctx, is, node.Node, builder.GetVisitInfo())
if err := core.CheckPrivilege(sctx.GetSessionVars().ActiveRoles, pm, visitInfo); err != nil {
return nil, nil, false, err
}
}

if err := core.CheckTableLock(sctx, is, builder.GetVisitInfo()); err != nil {
return nil, nil, false, err
}

if err := core.CheckTableMode(node); err != nil {
return nil, nil, false, err
}
*checked = true
}

// Handle the non-logical plan statement.
logic, isLogicalPlan := p.(base.LogicalPlan)
if !isLogicalPlan {
builder.HandleUnusedViewHints()
return p, names, true, nil
}

core.RecheckCTE(logic)

// todo: also you can customize each round's special logical opt flag here (like decorrelate rule or not)
finalPlan, cost, err := core.DoOptimize(ctx, sctx, builder.GetOptFlag(), logic)
if err != nil {
return nil, nil, false, err
}
builder.HandleUnusedViewHints()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guess we should have more fine-grained handling of HandleUnusedViewHints() , because multiple round of calling this function may cause many warnings.(But I think we can leave a TODO here and refine it later)
And also we should put it in derfer like the old code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. The defer part is addressed in bc1b396 (defer builder.HandleUnusedViewHints()), and I added a follow-up TODO in 7bb7149 to refine multi-round warning handling so only winner-round unused view-hint warnings are emitted when buildRound > 1.


if *bestPlan == nil || cost < *bestCost {
*bestCost = cost
*bestPlan = finalPlan
*bestNames = names
if needRestoreLogicalPlanCtx {
*bestLogicalPlanCtx = saveLogicalPlanBuildCtx(sctx.GetSessionVars())
}
}
return p, names, false, nil
}

// optimizeCnt is a global variable only used for test.
var optimizeCnt int

Expand All @@ -457,53 +546,60 @@ func optimize(ctx context.Context, sctx planctx.PlanContext, node *resolve.NodeW
})
sessVars := sctx.GetSessionVars()

// build logical plan
// Build the logical plan from the raw AST. The hint processor only keeps
// AST-derived metadata; per-build state is allocated inside PlanBuilder.
hintProcessor := hint.NewQBHintHandler(sctx.GetSessionVars().StmtCtx)
node.Node.Accept(hintProcessor)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
builder := planBuilderPool.Get().(*core.PlanBuilder)
defer planBuilderPool.Put(builder.ResetForReuse())
builder.Init(sctx, is, hintProcessor)
defer builder.HandleUnusedViewHints()
p, err := buildLogicalPlan(ctx, sctx, node, builder)
if err != nil {
return nil, nil, 0, err

// build multi logical plan from raw AST.
var (
buildRound = 1
needRestoreLogicalPlanCtx = buildRound > 1
bestCost = math.MaxFloat64
bestPlan base.PhysicalPlan
bestNames types.NameSlice
bestLogicalPlanCtx logicalPlanBuildCtx
checked bool
)
var initialLogicalPlanCtx logicalPlanBuildCtx
if needRestoreLogicalPlanCtx {
initialLogicalPlanCtx = saveLogicalPlanBuildCtx(sessVars)
}
beginOpt := time.Now()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Optimize_time now includes rewrite/build time

Why: DurationOptimizer.Total is now started before buildLogicalPlan(), so slow-log Optimize_time necessarily overlaps Rewrite_time, misleading diagnostics and slow-log rule matching that treats the two as disjoint phases.

Evidence: beginOpt := time.Now() at pkg/planner/optimize.go:497 is set before buildLogicalPlan(...) is called; sessVars.DurationOptimizer.Total = time.Since(beginOpt) at :583 captures the full window including rewrite. Slow log prints both Rewrite_time (slow_log.go:398) and Optimize_time (slow_log.go:408) independently — and slow-log rule matching keys on DurationOptimizer.Total (slow_log.go:756). Previously beginOpt was set after the build step, keeping the two intervals disjoint.

Suggestion: Move beginOpt := time.Now() to after the final buildLogicalPlan(...) call (i.e. just before core.DoOptimize(...)), restoring the original semantics where Optimize_time covers only the optimization phase.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in f89fdd9. Optimize_time now starts right before the first core.DoOptimize(...) call (not before logical build/rewrite), and is still recorded through defer only when optimization actually starts.

Copy link
Copy Markdown

@pantheon-ai pantheon-ai Bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AilinKid I've received your follow-up on this review comment and will continue working on it. I'll update this comment when I have something to share.

ℹ️ Learn more details on Pantheon AI.

for i := range buildRound {
if needRestoreLogicalPlanCtx && i > 0 {
restoreLogicalPlanBuildCtx(sessVars, initialLogicalPlanCtx)
}

activeRoles := sessVars.ActiveRoles
// Check privilege. Maybe it's better to move this to the Preprocess, but
// we need the table information to check privilege, which is collected
// into the visitInfo in the logical plan builder.
if pm := privilege.GetPrivilegeManager(sctx); pm != nil {
visitInfo := core.VisitInfo4PrivCheck(ctx, is, node.Node, builder.GetVisitInfo())
if err := core.CheckPrivilege(activeRoles, pm, visitInfo); err != nil {
p, names, nonLogical, err := buildAndOptimizeLogicalPlanRound(
ctx,
sctx,
node,
is,
hintProcessor,
&checked,
needRestoreLogicalPlanCtx,
&bestPlan,
&bestNames,
&bestCost,
&bestLogicalPlanCtx,
)
if err != nil {
return nil, nil, 0, err
}
if nonLogical {
// keep compatible with the old.
return p, names, 0, nil
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

if err := core.CheckTableLock(sctx, is, builder.GetVisitInfo()); err != nil {
return nil, nil, 0, err
}

if err := core.CheckTableMode(node); err != nil {
return nil, nil, 0, err
if bestPlan == nil {
return nil, nil, 0, errors.New("failed to build logical plan")
}

names := p.OutputNames()

// Handle the non-logical plan statement.
logic, isLogicalPlan := p.(base.LogicalPlan)
if !isLogicalPlan {
return p, names, 0, nil
if needRestoreLogicalPlanCtx {
restoreLogicalPlanBuildCtx(sessVars, bestLogicalPlanCtx)
}

core.RecheckCTE(logic)

beginOpt := time.Now()
finalPlan, cost, err := core.DoOptimize(ctx, sctx, builder.GetOptFlag(), logic)
// TODO: capture plan replayer here if it matches sql and plan digest

sessVars.DurationOptimizer.Total = time.Since(beginOpt)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old impl will update DurationOptimizer even got error, but new impl will not. So better put this in defer

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in bc1b396. DurationOptimizer.Total is now assigned in a deferred function, so it is recorded even when optimize returns with error.

return finalPlan, names, cost, err
return bestPlan, bestNames, bestCost, nil
}

// OptimizeExecStmt to handle the "execute" statement
Expand Down