Skip to content

Commit

Permalink
sql/opt/norm: propagate kv errors from cast folding
Browse files Browse the repository at this point in the history
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
  • Loading branch information
ajwerner authored and mgartner committed Sep 8, 2022
1 parent d33e93f commit b075b30
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 0 deletions.
1 change: 1 addition & 0 deletions pkg/sql/opt/norm/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
19 changes: 19 additions & 0 deletions pkg/sql/opt/norm/fold_constants_funcs.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}

Expand All @@ -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
}

Expand Down

0 comments on commit b075b30

Please sign in to comment.