From 82186d785fcac460d2b464785da3046591666f82 Mon Sep 17 00:00:00 2001 From: Michael Erickson Date: Tue, 8 Nov 2022 00:45:11 +0000 Subject: [PATCH] sql: catch panics in makeQueryIndexRecommendation In the optimizer we use the convention of panic-based error handling for a performance advantage. This means at the boundaries we must remember to recover and return errors instead of letting panics escape. `sql.(*optPlanningCtx).makeQueryIndexRecommendation` calls some optimizer helper functions in addition to calling `xform.(*Optimizer).Optimize`. While `Optimize` has the panic-recovery code, these helper functions do not, so we need to add panic recovery to `makeQueryIndexRecommendation` as well. Fixes: #88246 Release note: None --- pkg/sql/plan_opt.go | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/pkg/sql/plan_opt.go b/pkg/sql/plan_opt.go index 122ad36d9672..2a7a53631823 100644 --- a/pkg/sql/plan_opt.go +++ b/pkg/sql/plan_opt.go @@ -34,6 +34,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb" + "github.com/cockroachdb/cockroach/pkg/util/errorutil" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/tracing" "github.com/cockroachdb/errors" @@ -726,7 +727,24 @@ 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) error { +func (opc *optPlanningCtx) makeQueryIndexRecommendation(ctx context.Context) (err error) { + defer func() { + if r := recover(); r != nil { + // This code allows us to propagate internal errors without having to add + // error checks everywhere throughout the code. This is only possible + // because the code does not update shared state and does not manipulate + // locks. + if ok, e := errorutil.ShouldCatch(r); ok { + err = e + log.VEventf(ctx, 1, "%v", err) + } else { + // Other panic objects can't be considered "safe" and thus are + // propagated as crashes that terminate the session. + panic(r) + } + } + }() + // Save the normalized memo created by the optbuilder. savedMemo := opc.optimizer.DetachMemo(ctx) @@ -744,7 +762,7 @@ func (opc *optPlanningCtx) makeQueryIndexRecommendation(ctx context.Context) err opc.optimizer.NotifyOnMatchedRule(func(ruleName opt.RuleName) bool { return ruleName.IsNormalize() }) - if _, err := opc.optimizer.Optimize(); err != nil { + if _, err = opc.optimizer.Optimize(); err != nil { return err } @@ -762,7 +780,7 @@ func (opc *optPlanningCtx) makeQueryIndexRecommendation(ctx context.Context) err f.CopyWithoutAssigningPlaceholders, ) opc.optimizer.Memo().Metadata().UpdateTableMeta(hypTables) - if _, err := opc.optimizer.Optimize(); err != nil { + if _, err = opc.optimizer.Optimize(); err != nil { return err }