From b075b30df3b1e917f122a4cebc501f16ac57fafb Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Thu, 8 Sep 2022 10:38:42 -0400 Subject: [PATCH] sql/opt/norm: propagate kv errors from cast folding Swallowing KV errors here leads to incorrect results. Writes can be missed and serializability can be silently violated. This comes up in the context of the randomized schema change testing. Release note: None --- pkg/sql/opt/norm/BUILD.bazel | 1 + pkg/sql/opt/norm/fold_constants_funcs.go | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/pkg/sql/opt/norm/BUILD.bazel b/pkg/sql/opt/norm/BUILD.bazel index 9f02138a9f28..4c4dcb59f1b7 100644 --- a/pkg/sql/opt/norm/BUILD.bazel +++ b/pkg/sql/opt/norm/BUILD.bazel @@ -32,6 +32,7 @@ go_library( importpath = "github.com/cockroachdb/cockroach/pkg/sql/opt/norm", visibility = ["//visibility:public"], deps = [ + "//pkg/roachpb", "//pkg/sql/catalog/colinfo", "//pkg/sql/opt", "//pkg/sql/opt/cat", diff --git a/pkg/sql/opt/norm/fold_constants_funcs.go b/pkg/sql/opt/norm/fold_constants_funcs.go index 4fac21b82cfc..1b29c0b97164 100644 --- a/pkg/sql/opt/norm/fold_constants_funcs.go +++ b/pkg/sql/opt/norm/fold_constants_funcs.go @@ -11,6 +11,7 @@ package norm import ( + "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/sql/opt" "github.com/cockroachdb/cockroach/pkg/sql/opt/cat" "github.com/cockroachdb/cockroach/pkg/sql/opt/memo" @@ -369,6 +370,15 @@ func (c *CustomFuncs) FoldCast(input opt.ScalarExpr, typ *types.T) (_ opt.Scalar result, err := eval.Expr(c.f.evalCtx, texpr) if err != nil { + // Casts can require KV operations. KV errors are not safe to swallow. + // Check if the error is a KV error, and, if so, propagate it rather + // than swallowing it. See #85677. + // TODO(mgartner): Ideally, casts that can error and cause adverse + // side-effects would be marked as volatile so that they are not folded. + // That would eliminate the need for this special error handling. + if errors.HasInterface(err, (*roachpb.ErrorDetailInterface)(nil)) { + panic(err) + } return nil, false } @@ -395,6 +405,15 @@ func (c *CustomFuncs) FoldAssignmentCast( datum := memo.ExtractConstDatum(input) result, err := eval.PerformAssignmentCast(c.f.evalCtx, datum, typ) if err != nil { + // Casts can require KV operations. KV errors are not safe to swallow. + // Check if the error is a KV error, and, if so, propagate it rather + // than swallowing it. See #85677. + // TODO(mgartner): Ideally, casts that can error and cause adverse + // side-effects would be marked as volatile so that they are not folded. + // That would eliminate the need for this special error handling. + if errors.HasInterface(err, (*roachpb.ErrorDetailInterface)(nil)) { + panic(err) + } return nil, false }