Merge #30575

30575: sql, util: turn a panic into an UnexpectedWithIssueErr r=andreimatei a=andreimatei

In #26687 we're observing that sometimes a schema change overwrites an
already existing error, which is a panic. It's unclear what causes this;
the schema change should not be running at all if there's been an error.
This patch traps attempts to run schema changes after an error occured
and returns a conn-closing error, it logs it asking the user to
contribute to the respective issue, and also sends a sentry report.

Release note: None

Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>

pkg/sql/conn_executor.go

@@ -41,6 +41,7 @@ import (
  "github.com/cockroachdb/cockroach/pkg/storage/engine/enginepb"
  "github.com/cockroachdb/cockroach/pkg/util"
  "github.com/cockroachdb/cockroach/pkg/util/envutil"
+ "github.com/cockroachdb/cockroach/pkg/util/errorutil"
  "github.com/cockroachdb/cockroach/pkg/util/fsm"
  "github.com/cockroachdb/cockroach/pkg/util/hlc"
  "github.com/cockroachdb/cockroach/pkg/util/log"
@@ -331,7 +332,7 @@ func (s *Server) recordError(err error) {
  }
  }
  } else {
- typ := util.ErrorSource(err)
+ typ := log.ErrorSource(err)
  if typ == "" {
  typ = "unknown"
  }
@@ -1866,6 +1867,17 @@ func (ex *connExecutor) txnStateTransitionsApplyWrapper(
  case noEvent:
  case txnStart:
  case txnCommit:
+ if res.Err() != nil {
+ err := errorutil.UnexpectedWithIssueErrorf(
+ 26687,
+ "programming error: non-error event "+
+ advInfo.txnEvent.String()+ //the event is included like this so that it doesn't get sanitized
+ " generated even though res.Err() has been set to: %s",
+ res.Err())
+ log.Error(ex.Ctx(), err)
+ err.(errorutil.UnexpectedWithIssueErr).SendReport(ex.Ctx(), &ex.server.cfg.Settings.SV)
+ return advanceInfo{}, err
+ }
  if schemaChangeErr := ex.extraTxnState.schemaChangers.execSchemaChanges(
  ex.Ctx(), ex.server.cfg, &ex.sessionTracing,
  ); schemaChangeErr != nil {

pkg/sql/distsql_physical_planner.go

@@ -39,6 +39,7 @@ import (
  "github.com/cockroachdb/cockroach/pkg/util"
  "github.com/cockroachdb/cockroach/pkg/util/encoding"
  "github.com/cockroachdb/cockroach/pkg/util/envutil"
+ "github.com/cockroachdb/cockroach/pkg/util/errorutil"
  "github.com/cockroachdb/cockroach/pkg/util/log"
  "github.com/cockroachdb/cockroach/pkg/util/stop"
  "github.com/cockroachdb/cockroach/pkg/util/uuid"
@@ -516,7 +517,7 @@ func (p *PlanningCtx) sanityCheckAddresses() error {
  inverted := make(map[string]roachpb.NodeID)
  for nodeID, addr := range p.NodeAddresses {
  if otherNodeID, ok := inverted[addr]; ok {
- return util.UnexpectedWithIssueErrorf(
+ return errorutil.UnexpectedWithIssueErrorf(
  12876,
  "different nodes %d and %d with the same address '%s'", nodeID, otherNodeID, addr)
  }

pkg/util/error.go → pkg/util/errorutil/error.go

@@ -12,12 +12,14 @@
 // implied. See the License for the specific language governing
 // permissions and limitations under the License.
 
-package util
+package errorutil
 
 import (
+ "context"
  "fmt"
 
- "github.com/pkg/errors"
+ "github.com/cockroachdb/cockroach/pkg/settings"
+ "github.com/cockroachdb/cockroach/pkg/util/log"
 )
 
 // UnexpectedWithIssueErr indicates an error with an associated Github issue.
@@ -28,41 +30,39 @@ import (
 //
 // Modeled after pgerror.Unimplemented.
 type UnexpectedWithIssueErr struct {
- issue int
- msg string
+ issue int
+ msg string
+ safeMsg string
 }
 
 // UnexpectedWithIssueErrorf constructs an UnexpectedWithIssueError with the
 // provided issue and formatted message.
 func UnexpectedWithIssueErrorf(issue int, format string, args ...interface{}) error {
  return UnexpectedWithIssueErr{
- issue: issue,
- msg: fmt.Sprintf(format, args...),
+ issue: issue,
+ msg: fmt.Sprintf(format, args...),
+ safeMsg: log.ReportablesToSafeError(2 /* depth */, format, args).Error(),
  }
 }
 
 // Error implements the error interface.
 func (e UnexpectedWithIssueErr) Error() string {
- var fmtMsg string
- if e.msg != "" {
- fmtMsg = fmt.Sprintf(": %s", e.msg)
- }
- return fmt.Sprintf("unexpected error%s (we've been trying to track this particular issue down; "+
- "please report your reproduction at "+
- "https://github.com/cockroachdb/cockroach/issues/%d)", fmtMsg, e.issue)
+ return fmt.Sprintf("unexpected error: %s\nWe've been trying to track this particular issue down. "+
+ "Please report your reproduction at "+
+ "https://github.com/cockroachdb/cockroach/issues/%d "+
+ "unless that issue seems to have been resolved "+
+ "(in which case you might want to update crdb to a newer version).",
+ e.msg, e.issue)
 }
 
-// ErrorSource attempts to return the file:line where `i` was created if `i` has
-// that information (i.e. if it is an errors.withStack). Returns "" otherwise.
-func ErrorSource(i interface{}) string {
- type stackTracer interface {
- StackTrace() errors.StackTrace
- }
- if e, ok := i.(stackTracer); ok {
- tr := e.StackTrace()
- if len(tr) > 0 {
- return fmt.Sprintf("%v", tr[0]) // prints file:line
- }
- }
- return ""
+// SafeMessage implements the SafeMessager interface.
+func (e UnexpectedWithIssueErr) SafeMessage() string {
+ return fmt.Sprintf("issue #%d: %s", e.issue, e.safeMsg)
+}
+
+// SendReport creates a Sentry report about the error, if the settings allow.
+// The format string will be reproduced ad litteram in the report; the arguments
+// will be sanitized.
+func (e UnexpectedWithIssueErr) SendReport(ctx context.Context, sv *settings.Values) {
+ log.SendCrashReport(ctx, sv, 1 /* depth */, "%s", []interface{}{e})
 }

pkg/util/error_test.go → pkg/util/errorutil/error_test.go

@@ -12,14 +12,26 @@
 // implied. See the License for the specific language governing
 // permissions and limitations under the License.
 
-package util
+package errorutil
 
-import "testing"
+import (
+ "testing"
+)
 
 func TestUnexpectedWithIssueErrorf(t *testing.T) {
  err := UnexpectedWithIssueErrorf(1234, "args: %d %s %f", 1, "two", 3.0)
- exp := "unexpected error: args: 1 two 3.000000 (we've been trying to track this particular issue down; please report your reproduction at https://github.com/cockroachdb/cockroach/issues/1234)"
+ exp := "unexpected error: args: 1 two 3.000000\n" +
+ "We've been trying to track this particular issue down. Please report your " +
+ "reproduction at https://github.com/cockroachdb/cockroach/issues/1234 unless " +
+ "that issue seems to have been resolved (in which case you might want to " +
+ "update crdb to a newer version)."
  if err.Error() != exp {
  t.Errorf("Expected message:\n %s\ngot:\n %s", exp, err.Error())
  }
+
+ safeMsg := err.(UnexpectedWithIssueErr).SafeMessage()
+ exp = "issue #1234: error_test.go:22: args: %d %s %f | int; string; float64"
+ if safeMsg != exp {
+ t.Errorf("Expected SafeMessage:\n%s\ngot:\n%s", exp, safeMsg)
+ }
 }

pkg/util/log/crash_reporting.go

@@ -292,7 +292,7 @@ func (e *safeError) Error() string {
 // anonymized reporting.
 func Redact(r interface{}) string {
  typAnd := func(i interface{}, text string) string {
- typ := util.ErrorSource(i)
+ typ := ErrorSource(i)
  if typ == "" {
  typ = fmt.Sprintf("%T", i)
  }
@@ -518,3 +518,18 @@ var tagFns []tagFn
 func RegisterTagFn(key string, value func(context.Context) string) {
  tagFns = append(tagFns, tagFn{key, value})
 }
+
+// ErrorSource attempts to return the file:line where `i` was created if `i` has
+// that information (i.e. if it is an errors.withStack). Returns "" otherwise.
+func ErrorSource(i interface{}) string {
+ type stackTracer interface {
+ StackTrace() errors.StackTrace
+ }
+ if e, ok := i.(stackTracer); ok {
+ tr := e.StackTrace()
+ if len(tr) > 0 {
+ return fmt.Sprintf("%v", tr[0]) // prints file:line
+ }
+ }
+ return ""
+}