Skip to content

Commit

Permalink
sem/builtins: vectorized engine no longer catches crdb_internal.force…
Browse files Browse the repository at this point in the history
…_panic

Previously, when executing `crdb_internal.force_panic` builtin, if it
was executed via the vectorized engine, we would catch the panic and
convert it into an internal error instead. This is undesirable and is
now fixed.

Release note: None
  • Loading branch information
yuzefovich committed Apr 6, 2021
1 parent 15aee6a commit ae6233f
Show file tree
Hide file tree
Showing 5 changed files with 23 additions and 14 deletions.
17 changes: 9 additions & 8 deletions pkg/sql/colexecerror/error.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,10 +122,10 @@ func shouldCatchPanic(panicEmittedFrom string) bool {
// unchanged by the higher-level catchers.
return false
}
const nonVectorizedTestPrefix = "github.com/cockroachdb/cockroach/pkg/sql/colexecerror.NonVectorizedTestPanic"
if strings.HasPrefix(panicEmittedFrom, nonVectorizedTestPrefix) {
// This panic came from NonVectorizedTestPanic() method and should not
// be caught for testing purposes.
const nonCatchablePanicPrefix = "github.com/cockroachdb/cockroach/pkg/sql/colexecerror.NonCatchablePanic"
if strings.HasPrefix(panicEmittedFrom, nonCatchablePanicPrefix) {
// This panic came from NonCatchablePanic() method and should not be
// caught.
return false
}
return strings.HasPrefix(panicEmittedFrom, colPackagesPrefix) ||
Expand Down Expand Up @@ -200,9 +200,10 @@ func ExpectedError(err error) {
panic(newNotInternalError(err))
}

// NonVectorizedTestPanic is the equivalent of Golang's 'panic' word that should
// be used by the testing code within the vectorized engine to simulate a panic
// that occurs outside of the engine (and, thus, should not be caught).
func NonVectorizedTestPanic(object interface{}) {
// NonCatchablePanic is the equivalent of Golang's 'panic' word that can be used
// in order to crash the goroutine. It could be used by the testing code within
// the vectorized engine to simulate a panic that occurs outside of the engine
// (and, thus, should not be caught).
func NonCatchablePanic(object interface{}) {
panic(object)
}
10 changes: 5 additions & 5 deletions pkg/sql/colexecerror/error_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func TestCatchVectorizedRuntimeError(t *testing.T) {
require.Panics(t, func() {
require.NoError(t, colexecerror.CatchVectorizedRuntimeError(func() {
require.NoError(t, colexecerror.CatchVectorizedRuntimeError(func() {
colexecerror.NonVectorizedTestPanic(errors.New("should not be caught"))
colexecerror.NonCatchablePanic(errors.New("should not be caught"))
}))
}))
})
Expand Down Expand Up @@ -62,15 +62,15 @@ func TestCatchVectorizedRuntimeError(t *testing.T) {
require.False(t, strings.Contains(notAnnotatedErr.Error(), annotationText))
}

// TestNonVectorizedTestPanicIsNotCaught verifies that panics emitted via
// NonVectorizedTestPanic() method are not caught by the catcher.
func TestNonVectorizedTestPanicIsNotCaught(t *testing.T) {
// TestNonCatchablePanicIsNotCaught verifies that panics emitted via
// NonCatchablePanic() method are not caught by the catcher.
func TestNonCatchablePanicIsNotCaught(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

require.Panics(t, func() {
require.NoError(t, colexecerror.CatchVectorizedRuntimeError(func() {
colexecerror.NonVectorizedTestPanic("should panic")
colexecerror.NonCatchablePanic("should panic")
}))
})
}
2 changes: 1 addition & 1 deletion pkg/sql/colflow/vectorized_panic_propagation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ func (e *testNonVectorizedPanicEmitter) Init() {
func (e *testNonVectorizedPanicEmitter) Next(ctx context.Context) coldata.Batch {
if !e.emitBatch {
e.emitBatch = true
colexecerror.NonVectorizedTestPanic("")
colexecerror.NonCatchablePanic("")
}

e.emitBatch = false
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/sem/builtins/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ go_library(
"//pkg/sql/catalog/catalogkv",
"//pkg/sql/catalog/catconstants",
"//pkg/sql/catalog/descpb",
"//pkg/sql/colexecerror",
"//pkg/sql/lex",
"//pkg/sql/lexbase",
"//pkg/sql/paramparse",
Expand Down
7 changes: 7 additions & 0 deletions pkg/sql/sem/builtins/builtins.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/colexecerror"
"github.com/cockroachdb/cockroach/pkg/sql/lex"
"github.com/cockroachdb/cockroach/pkg/sql/lexbase"
"github.com/cockroachdb/cockroach/pkg/sql/parser"
Expand Down Expand Up @@ -4076,6 +4077,12 @@ may increase either contention or retry errors, or both.`,
return nil, err
}
msg := string(*args[0].(*tree.DString))
// Use a special method to panic in order to go around the
// vectorized panic-catcher (which would catch the panic from
// Golang's 'panic' and would convert it into an internal
// error).
colexecerror.NonCatchablePanic(msg)
// This code is unreachable.
panic(msg)
},
Info: "This function is used only by CockroachDB's developers for testing purposes.",
Expand Down

0 comments on commit ae6233f

Please sign in to comment.