Skip to content

Commit

Permalink
sql: set index recommendations after planning but before execution
Browse files Browse the repository at this point in the history
This commit moves the call to set the index recommendations to be done
right after planning was completed. Previously, it was done after the
execution, but it makes more sense to do it after planning. This also
allows us to remove the check on the txn still being open.

This required clarifying how `instrumentationHelper.indexRecs` is used.
Previously, it was used for two purposes:
- for recording recommendations to be included in the
`statement_statistics` system table
- for showing when executing EXPLAIN statement.

These two usages have somewhat different requirements, so this commit
splits them out into two different slices. This also allows us to reuse
the recommendations from the latter should we choose to generate the
recommendations for the former (previously, this would result in
redundant regeneration of the recommendations).

Epic: None

Release note: None
  • Loading branch information
yuzefovich committed Mar 21, 2023
1 parent 77029b1 commit d061da4
Showing 4 changed files with 41 additions and 35 deletions.
17 changes: 6 additions & 11 deletions pkg/sql/conn_executor_exec.go
Original file line number Diff line number Diff line change
@@ -1374,17 +1374,6 @@ func (ex *connExecutor) dispatchToExecutionEngine(

populateQueryLevelStatsAndRegions(ctx, planner, ex.server.cfg, &stats, &ex.cpuStatsCollector)

// The transaction (from planner.txn) may already have been committed at this point,
// due to one-phase commit optimization or an error. Since we use that transaction
// on the optimizer, check if is still open before generating index recommendations.
if planner.txn.IsOpen() {
// Set index recommendations, so it can be saved on statement statistics.
// TODO(yuzefovich): figure out whether we want to set isInternalPlanner
// to true for the internal executors.
isInternal := ex.executorType == executorTypeInternal || planner.isInternalPlanner
planner.instrumentation.SetIndexRecommendations(ctx, ex.server.idxRecommendationsCache, planner, isInternal)
}

// Record the statement summary. This also closes the plan if the
// plan has not been closed earlier.
stmtFingerprintID = ex.recordStatementSummary(
@@ -1683,6 +1672,12 @@ func (ex *connExecutor) makeExecPlan(ctx context.Context, planner *planner) erro
ex.extraTxnState.numDDL++
}

// Set index recommendations, so it can be saved on statement statistics.
// TODO(yuzefovich): figure out whether we want to set isInternalPlanner
// to true for the internal executors.
isInternal := ex.executorType == executorTypeInternal || planner.isInternalPlanner
planner.instrumentation.SetIndexRecommendations(ctx, ex.server.idxRecommendationsCache, planner, isInternal)

return nil
}

2 changes: 1 addition & 1 deletion pkg/sql/explain_plan.go
Original file line number Diff line number Diff line change
@@ -139,7 +139,7 @@ func (e *explainPlanNode) startExec(params runParams) error {
}
}
// Add index recommendations to output, if they exist.
if recs := params.p.instrumentation.indexRecs; recs != nil {
if recs := params.p.instrumentation.explainIndexRecs; recs != nil {
// First add empty row.
rows = append(rows, "")
rows = append(rows, fmt.Sprintf("index recommendations: %d", len(recs)))
38 changes: 22 additions & 16 deletions pkg/sql/instrumentation.go
Original file line number Diff line number Diff line change
@@ -151,10 +151,11 @@ type instrumentationHelper struct {
costEstimate float64

// indexRecs contains index recommendations for the planned statement. It
// will only be populated if the statement is an EXPLAIN statement, or if
// recommendations are requested for the statement for populating the
// statement_statistics table.
// will only be populated if recommendations are requested for the statement
// for populating the statement_statistics table.
indexRecs []indexrec.Rec
// explainIndexRecs contains index recommendations for EXPLAIN statements.
explainIndexRecs []indexrec.Rec

// maxFullScanRows is the maximum number of rows scanned by a full scan, as
// estimated by the optimizer.
@@ -789,24 +790,29 @@ func (ih *instrumentationHelper) SetIndexRecommendations(
stmtType,
isInternal,
) {
opc := &planner.optPlanningCtx
opc.reset(ctx)
f := opc.optimizer.Factory()
evalCtx := opc.p.EvalContext()
f.Init(ctx, evalCtx, opc.catalog)
f.FoldingControl().AllowStableFolds()
bld := optbuilder.New(ctx, &opc.p.semaCtx, evalCtx, opc.catalog, f, opc.p.stmt.AST)
err := bld.Build()
if err != nil {
log.Warningf(ctx, "unable to build memo: %s", err)
// If the statement is an EXPLAIN, then we might have already generated
// the index recommendations. If so, we can skip generation here.
if len(ih.explainIndexRecs) > 0 {
recommendations = ih.explainIndexRecs
} else {
err = opc.makeQueryIndexRecommendation(ctx)
opc := &planner.optPlanningCtx
opc.reset(ctx)
f := opc.optimizer.Factory()
evalCtx := opc.p.EvalContext()
f.Init(ctx, evalCtx, opc.catalog)
f.FoldingControl().AllowStableFolds()
bld := optbuilder.New(ctx, &opc.p.semaCtx, evalCtx, opc.catalog, f, opc.p.stmt.AST)
err := bld.Build()
if err != nil {
log.Warningf(ctx, "unable to generate index recommendations: %s", err)
log.Warningf(ctx, "unable to build memo: %s", err)
} else {
recommendations, err = opc.makeQueryIndexRecommendation(ctx)
if err != nil {
log.Warningf(ctx, "unable to generate index recommendations: %s", err)
}
}
}
reset = true
recommendations = ih.indexRecs
}
ih.indexRecs = idxRec.UpdateIndexRecommendations(
ih.fingerprint,
19 changes: 12 additions & 7 deletions pkg/sql/plan_opt.go
Original file line number Diff line number Diff line change
@@ -578,9 +578,11 @@ func (opc *optPlanningCtx) buildExecMemo(ctx context.Context) (_ *memo.Memo, _ e
// find potential index candidates in the memo.
_, isExplain := opc.p.stmt.AST.(*tree.Explain)
if isExplain && p.SessionData().IndexRecommendationsEnabled {
if err := opc.makeQueryIndexRecommendation(ctx); err != nil {
indexRecs, err := opc.makeQueryIndexRecommendation(ctx)
if err != nil {
return nil, err
}
opc.p.instrumentation.explainIndexRecs = indexRecs
}

if _, isCanned := opc.p.stmt.AST.(*tree.CannedOptPlan); !isCanned {
@@ -721,7 +723,9 @@ func (p *planner) DecodeGist(gist string, external bool) ([]string, error) {
// indexes hypothetically added to the table. An index recommendation for the
// query is outputted based on which hypothetical indexes are helpful in the
// optimal plan.
func (opc *optPlanningCtx) makeQueryIndexRecommendation(ctx context.Context) (err error) {
func (opc *optPlanningCtx) makeQueryIndexRecommendation(
ctx context.Context,
) (_ []indexrec.Rec, err error) {
defer func() {
if r := recover(); r != nil {
// This code allows us to propagate internal errors without having to add
@@ -757,7 +761,7 @@ func (opc *optPlanningCtx) makeQueryIndexRecommendation(ctx context.Context) (er
return ruleName.IsNormalize()
})
if _, err = opc.optimizer.Optimize(); err != nil {
return err
return nil, err
}

// Walk through the fully normalized memo to determine index candidates and
@@ -775,12 +779,13 @@ func (opc *optPlanningCtx) makeQueryIndexRecommendation(ctx context.Context) (er
)
opc.optimizer.Memo().Metadata().UpdateTableMeta(f.EvalContext(), hypTables)
if _, err = opc.optimizer.Optimize(); err != nil {
return err
return nil, err
}

opc.p.instrumentation.indexRecs, err = indexrec.FindRecs(ctx, f.Memo().RootExpr(), f.Metadata())
var indexRecs []indexrec.Rec
indexRecs, err = indexrec.FindRecs(ctx, f.Memo().RootExpr(), f.Metadata())
if err != nil {
return err
return nil, err
}

// Re-initialize the optimizer (which also re-initializes the factory) and
@@ -794,5 +799,5 @@ func (opc *optPlanningCtx) makeQueryIndexRecommendation(ctx context.Context) (er
f.CopyWithoutAssigningPlaceholders,
)

return nil
return indexRecs, nil
}

0 comments on commit d061da4

Please sign in to comment.