opt: prevent apply-join panics from crashing nodes

mgartner · mgartner · commit 5c0a632100ba · 2022-09-21T16:10:41.000-04:00
Previously, it was possible for the execution of an apply-join to crash
a node due to an uncaught optimizer panic when calling the
`planRightSideFn` closure. This closure is invoked for every input row
to the apply-join. It replaces variables in the expression on the right
side of the join with constants using `Factory.CopyAndReplace`, which
can panic. This panic won't be caught by the panic-catching logic in
`Optimizer.Optimize` because the closure is invoked outside the context
of `Optimizer.Optimize` - it's occurring during execution instead.

This commit copies the panic-catching logic of `Optimizer.Optimize` to
the apply-join's `planRightSideFn` closure to ensure that any panics are
caught.

Release Note (bug fix): A bug has been fixed that could cause nodes to
crash in rare cases when executing apply-joins in query plans.
diff --git a/pkg/sql/opt/exec/execbuilder/BUILD.bazel b/pkg/sql/opt/exec/execbuilder/BUILD.bazel
@@ -48,6 +48,7 @@ go_library(
         "//pkg/util/encoding",
         "//pkg/util/errorutil",
         "//pkg/util/errorutil/unimplemented",
+        "//pkg/util/log",
         "//pkg/util/timeutil",
         "//pkg/util/treeprinter",
         "@com_github_cockroachdb_errors//:errors",
diff --git a/pkg/sql/opt/exec/execbuilder/relational.go b/pkg/sql/opt/exec/execbuilder/relational.go
@@ -41,7 +41,9 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/buildutil"
 	"github.com/cockroachdb/cockroach/pkg/util/encoding"
+	"github.com/cockroachdb/cockroach/pkg/util/errorutil"
 	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/redact"
@@ -1024,7 +1026,29 @@ func (b *Builder) buildApplyJoin(join memo.RelExpr) (execPlan, error) {
 	//
 	// Note: we put o outside of the function so we allocate it only once.
 	var o xform.Optimizer
-	planRightSideFn := func(ctx context.Context, ef exec.Factory, leftRow tree.Datums) (exec.Plan, error) {
+	planRightSideFn := func(ctx context.Context, ef exec.Factory, leftRow tree.Datums) (_ exec.Plan, 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.
+				//
+				// This is the same panic-catching logic that exists in
+				// o.Optimize() below. It's required here because it's possible
+				// for factory functions to panic below, like
+				// CopyAndReplaceDefault.
+				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)
+				}
+			}
+		}()
+
 		o.Init(ctx, b.evalCtx, b.catalog)
 		f := o.Factory()
 
