Improved error messages (tabletserver)

go/mysql/conn.go

@@ -897,7 +897,7 @@ func (c *Conn) handleNextCommand(handler Handler) bool {
 	default:
 		log.Errorf("Got unhandled packet (default) from %s, returning error: %v", c, data)
 		c.recycleReadPacket()
-		if !c.writeErrorAndLog(ERUnknownComError, SSUnknownComError, "command handling not implemented yet: %v", data[0]) {
+		if !c.writeErrorAndLog(ERUnknownComError, SSNetError, "command handling not implemented yet: %v", data[0]) {
 			return false
 		}
 	}
@@ -922,15 +922,15 @@ func (c *Conn) handleComStmtReset(data []byte) bool {
 	c.recycleReadPacket()
 	if !ok {
 		log.Error("Got unhandled packet from client %v, returning error: %v", c.ConnectionID, data)
-		if !c.writeErrorAndLog(ERUnknownComError, SSUnknownComError, "error handling packet: %v", data) {
+		if !c.writeErrorAndLog(ERUnknownComError, SSNetError, "error handling packet: %v", data) {
 			return false
 		}
 	}
 
 	prepare, ok := c.PrepareData[stmtID]
 	if !ok {
 		log.Error("Commands were executed in an improper order from client %v, packet: %v", c.ConnectionID, data)
-		if !c.writeErrorAndLog(CRCommandsOutOfSync, SSUnknownComError, "commands were executed in an improper order: %v", data) {
+		if !c.writeErrorAndLog(CRCommandsOutOfSync, SSNetError, "commands were executed in an improper order: %v", data) {
 			return false
 		}
 	}
@@ -1163,7 +1163,7 @@ func (c *Conn) handleComSetOption(data []byte) bool {
 			c.Capabilities &^= CapabilityClientMultiStatements
 		default:
 			log.Errorf("Got unhandled packet (ComSetOption default) from client %v, returning error: %v", c.ConnectionID, data)
-			if !c.writeErrorAndLog(ERUnknownComError, SSUnknownComError, "error handling packet: %v", data) {
+			if !c.writeErrorAndLog(ERUnknownComError, SSNetError, "error handling packet: %v", data) {
 				return false
 			}
 		}
@@ -1173,7 +1173,7 @@ func (c *Conn) handleComSetOption(data []byte) bool {
 		}
 	} else {
 		log.Errorf("Got unhandled packet (ComSetOption else) from client %v, returning error: %v", c.ConnectionID, data)
-		if !c.writeErrorAndLog(ERUnknownComError, SSUnknownComError, "error handling packet: %v", data) {
+		if !c.writeErrorAndLog(ERUnknownComError, SSNetError, "error handling packet: %v", data) {
 			return false
 		}
 	}

go/mysql/constants.go

@@ -516,6 +516,9 @@ const (
 	ERDataTooLong                  = 1406
 	ERForbidSchemaChange           = 1450
 	ERDataOutOfRange               = 1690
+
+	// server not available
+	ERServerIsntAvailable = 3168
 )
 
 // Sql states for errors.
@@ -527,9 +530,6 @@ const (
 	// in client.c. So using that one.
 	SSUnknownSQLState = "HY000"
 
-	// SSUnknownComError is ER_UNKNOWN_COM_ERROR
-	SSUnknownComError = "08S01"
-
 	// SSNetError is network related error
 	SSNetError = "08S01"
 

go/mysql/server_test.go

@@ -565,7 +565,7 @@ func TestServer(t *testing.T) {
 
 	// If there's an error after streaming has started,
 	// we should get a 2013
-	th.SetErr(NewSQLError(ERUnknownComError, SSUnknownComError, "forced error after send"))
+	th.SetErr(NewSQLError(ERUnknownComError, SSNetError, "forced error after send"))
 	output, err = runMysqlWithErr(t, params, "error after send")
 	require.Error(t, err)
 	assert.Contains(t, output, "ERROR 2013 (HY000)", "Unexpected output for 'panic'")
@@ -650,7 +650,7 @@ func TestServerStats(t *testing.T) {
 	connRefuse.Reset()
 
 	// Run an 'error' command.
-	th.SetErr(NewSQLError(ERUnknownComError, SSUnknownComError, "forced query error"))
+	th.SetErr(NewSQLError(ERUnknownComError, SSNetError, "forced query error"))
 	output, ok := runMysql(t, params, "error")
 	require.False(t, ok, "mysql should have failed: %v", output)
 

go/mysql/sql_error.go

@@ -92,46 +92,20 @@ func NewSQLErrorFromError(err error) error {
 	}
 
 	sErr := convertToMysqlError(err)
-	if _, ok := sErr.(*SQLError); ok {
-		return sErr
+	if serr, ok := sErr.(*SQLError); ok {
+		return serr
 	}
 
 	msg := err.Error()
 	match := errExtract.FindStringSubmatch(msg)
-	if len(match) < 2 {
-		// Map vitess error codes into the mysql equivalent
-		code := vterrors.Code(err)
-		num := ERUnknownError
-		ss := SSUnknownSQLState
-		switch code {
-		case vtrpcpb.Code_CANCELED, vtrpcpb.Code_DEADLINE_EXCEEDED, vtrpcpb.Code_ABORTED:
-			num = ERQueryInterrupted
-			ss = SSQueryInterrupted
-		case vtrpcpb.Code_UNKNOWN, vtrpcpb.Code_INVALID_ARGUMENT, vtrpcpb.Code_NOT_FOUND, vtrpcpb.Code_ALREADY_EXISTS,
-			vtrpcpb.Code_FAILED_PRECONDITION, vtrpcpb.Code_OUT_OF_RANGE, vtrpcpb.Code_UNAVAILABLE, vtrpcpb.Code_DATA_LOSS:
-			num = ERUnknownError
-		case vtrpcpb.Code_PERMISSION_DENIED, vtrpcpb.Code_UNAUTHENTICATED:
-			num = ERAccessDeniedError
-			ss = SSAccessDeniedError
-		case vtrpcpb.Code_RESOURCE_EXHAUSTED:
-			num = demuxResourceExhaustedErrors(err.Error())
-			ss = SSClientError
-		case vtrpcpb.Code_UNIMPLEMENTED:
-			num = ERNotSupportedYet
-			ss = SSClientError
-		case vtrpcpb.Code_INTERNAL:
-			num = ERInternalError
-			ss = SSUnknownSQLState
-		}
-
-		// Not found, build a generic SQLError.
-		return &SQLError{
-			Num:     num,
-			State:   ss,
-			Message: msg,
-		}
+	if len(match) >= 2 {
+		return extractSQLErrorFromMessage(match, msg)
 	}
 
+	return mapToSQLErrorFromErrorCode(err, msg)
+}
+
+func extractSQLErrorFromMessage(match []string, msg string) *SQLError {
 	num, err := strconv.Atoi(match[1])
 	if err != nil {
 		return &SQLError{
@@ -141,12 +115,44 @@ func NewSQLErrorFromError(err error) error {
 		}
 	}
 
-	serr := &SQLError{
+	return &SQLError{
 		Num:     num,
 		State:   match[2],
 		Message: msg,
 	}
-	return serr
+}
+
+func mapToSQLErrorFromErrorCode(err error, msg string) *SQLError {
+	// Map vitess error codes into the mysql equivalent
+	num := ERUnknownError
+	ss := SSUnknownSQLState
+	switch vterrors.Code(err) {
+	case vtrpcpb.Code_CANCELED, vtrpcpb.Code_DEADLINE_EXCEEDED, vtrpcpb.Code_ABORTED:
+		num = ERQueryInterrupted
+		ss = SSQueryInterrupted
+	case vtrpcpb.Code_UNKNOWN, vtrpcpb.Code_INVALID_ARGUMENT, vtrpcpb.Code_NOT_FOUND, vtrpcpb.Code_ALREADY_EXISTS,
+		vtrpcpb.Code_FAILED_PRECONDITION, vtrpcpb.Code_OUT_OF_RANGE, vtrpcpb.Code_UNAVAILABLE, vtrpcpb.Code_DATA_LOSS:
+		num = ERUnknownError
+	case vtrpcpb.Code_PERMISSION_DENIED, vtrpcpb.Code_UNAUTHENTICATED:
+		num = ERAccessDeniedError
+		ss = SSAccessDeniedError
+	case vtrpcpb.Code_RESOURCE_EXHAUSTED:
+		num = demuxResourceExhaustedErrors(err.Error())
+		ss = SSClientError
+	case vtrpcpb.Code_UNIMPLEMENTED:
+		num = ERNotSupportedYet
+		ss = SSClientError
+	case vtrpcpb.Code_INTERNAL:
+		num = ERInternalError
+		ss = SSUnknownSQLState
+	}
+
+	// Not found, build a generic SQLError.
+	return &SQLError{
+		Num:     num,
+		State:   ss,
+		Message: msg,
+	}
 }
 
 var stateToMysqlCode = map[vterrors.State]struct {
@@ -182,6 +188,9 @@ var stateToMysqlCode = map[vterrors.State]struct {
 	vterrors.WrongNumberOfColumnsInSelect: {num: ERWrongNumberOfColumnsInSelect, state: SSWrongNumberOfColumns},
 	vterrors.WrongTypeForVar:              {num: ERWrongTypeForVar, state: SSClientError},
 	vterrors.WrongValueForVar:             {num: ERWrongValueForVar, state: SSClientError},
+	vterrors.ServerNotAvailable:           {num: ERServerIsntAvailable, state: SSNetError},
+	vterrors.CantDoThisInTransaction:      {num: ERCantDoThisDuringAnTransaction, state: SSCantDoThisDuringAnTransaction},
+	vterrors.NoSuchSession:                {num: ERUnknownComError, state: SSNetError},
 }
 
 func init() {

go/mysql/sql_error_test.go

@@ -19,8 +19,6 @@ package mysql
 import (
 	"testing"
 
-	"github.com/stretchr/testify/require"
-
 	"vitess.io/vitess/go/vt/proto/vtrpc"
 	"vitess.io/vitess/go/vt/vterrors"
 
@@ -154,11 +152,9 @@ func TestNewSQLErrorFromError(t *testing.T) {
 
 	for _, tc := range tCases {
 		t.Run(tc.err.Error(), func(t *testing.T) {
-			err := NewSQLErrorFromError(tc.err)
-			sErr, ok := err.(*SQLError)
-			require.True(t, ok)
-			assert.Equal(t, tc.num, sErr.Number())
-			assert.Equal(t, tc.ss, sErr.SQLState())
+			err := NewSQLErrorFromError(tc.err).(*SQLError)
+			assert.Equal(t, tc.num, err.Number())
+			assert.Equal(t, tc.ss, err.SQLState())
 		})
 	}
 }

go/test/utils/diff.go

@@ -17,6 +17,7 @@ limitations under the License.
 package utils
 
 import (
+	"reflect"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -43,11 +44,13 @@ import (
 // In Test*() function:
 //
 // mustMatch(t, want, got, "something doesn't match")
-func MustMatchFn(allowUnexportedTypes []interface{}, ignoredFields []string, extraOpts ...cmp.Option) func(t *testing.T, want, got interface{}, errMsg ...string) {
-	diffOpts := append([]cmp.Option{
-		cmp.AllowUnexported(allowUnexportedTypes...),
+func MustMatchFn(ignoredFields ...string) func(t *testing.T, want, got interface{}, errMsg ...string) {
+	diffOpts := []cmp.Option{
+		cmp.Exporter(func(reflect.Type) bool {
+			return true
+		}),
 		cmpIgnoreFields(ignoredFields...),
-	}, extraOpts...)
+	}
 	// Diffs want/got and fails with errMsg on any failure.
 	return func(t *testing.T, want, got interface{}, errMsg ...string) {
 		t.Helper()
@@ -62,7 +65,7 @@ func MustMatchFn(allowUnexportedTypes []interface{}, ignoredFields []string, ext
 // Usage in Test*() function:
 //
 // testutils.MustMatch(t, want, got, "something doesn't match")
-var MustMatch = MustMatchFn(nil, nil)
+var MustMatch = MustMatchFn()
 
 // Skips fields of pathNames for cmp.Diff.
 // Similar to standard cmpopts.IgnoreFields, but allows unexported fields.

go/vt/discovery/healthcheck_test.go

@@ -1232,9 +1232,4 @@ func createTestTablet(uid uint32, cell, host string) *topodatapb.Tablet {
 	return tablet
 }
 
-var mustMatch = utils.MustMatchFn(
-	[]interface{}{ // types with unexported fields
-		TabletHealth{},
-	},
-	[]string{".Conn"}, // ignored fields
-)
+var mustMatch = utils.MustMatchFn(".Conn" /* ignored fields*/)

go/vt/sqlparser/analyzer_test.go

@@ -476,9 +476,4 @@ func TestNewPlanValue(t *testing.T) {
 	}
 }
 
-var mustMatch = utils.MustMatchFn(
-	[]interface{}{ // types with unexported fields
-		sqltypes.Value{},
-	},
-	[]string{".Conn"}, // ignored fields
-)
+var mustMatch = utils.MustMatchFn(".Conn")

go/vt/vterrors/state.go

@@ -42,6 +42,7 @@ const (
 	NoDB
 	InnodbReadOnly
 	WrongNumberOfColumnsInSelect
+	CantDoThisInTransaction
 
 	// not found
 	BadDb
@@ -50,6 +51,7 @@ const (
 	SPDoesNotExist
 	UnknownSystemVariable
 	UnknownTable
+	NoSuchSession
 
 	// already exists
 	DbCreateExists
@@ -67,6 +69,9 @@ const (
 	// permission denied
 	AccessDeniedError
 
+	// server not available
+	ServerNotAvailable
+
 	// No state should be added below NumOfStates
 	NumOfStates
 )

go/vt/vtgate/engine/plan_description_test.go

@@ -86,16 +86,9 @@ func TestPlanDescriptionWithInputs(t *testing.T) {
 		Inputs: []PrimitiveDescription{routeDescr},
 	}
 
-	mustMatch(t, expected, planDescription, "descriptions did not match")
+	utils.MustMatch(t, expected, planDescription, "descriptions did not match")
 }
 
-var mustMatch = utils.MustMatchFn(
-	[]interface{}{ // types with unexported fields
-		sqltypes.Value{},
-	},
-	[]string{}, // ignored fields
-)
-
 func getDescriptionFor(route *Route) PrimitiveDescription {
 	return PrimitiveDescription{
 		OperatorType:      "Route",

go/vt/vtgate/evalengine/arithmetic_test.go

@@ -890,13 +890,6 @@ func TestToNative(t *testing.T) {
 	}
 }
 
-var mustMatch = utils.MustMatchFn(
-	[]interface{}{ // types with unexported fields
-		EvalResult{},
-	},
-	[]string{}, // ignored fields
-)
-
 func TestNewNumeric(t *testing.T) {
 	tcases := []struct {
 		v   sqltypes.Value
@@ -944,7 +937,7 @@ func TestNewNumeric(t *testing.T) {
 			continue
 		}
 
-		mustMatch(t, tcase.out, got, "newEvalResult")
+		utils.MustMatch(t, tcase.out, got, "newEvalResult")
 	}
 }
 
@@ -991,7 +984,7 @@ func TestNewIntegralNumeric(t *testing.T) {
 			continue
 		}
 
-		mustMatch(t, tcase.out, got, "newIntegralNumeric")
+		utils.MustMatch(t, tcase.out, got, "newIntegralNumeric")
 	}
 }
 
@@ -1047,7 +1040,7 @@ func TestAddNumeric(t *testing.T) {
 	for _, tcase := range tcases {
 		got := addNumeric(tcase.v1, tcase.v2)
 
-		mustMatch(t, tcase.out, got, "addNumeric")
+		utils.MustMatch(t, tcase.out, got, "addNumeric")
 	}
 }
 
@@ -1105,8 +1098,8 @@ func TestPrioritize(t *testing.T) {
 	for _, tcase := range tcases {
 		t.Run(tcase.v1.Value().String()+" - "+tcase.v2.Value().String(), func(t *testing.T) {
 			got1, got2 := makeNumericAndprioritize(tcase.v1, tcase.v2)
-			mustMatch(t, tcase.out1, got1, "makeNumericAndprioritize")
-			mustMatch(t, tcase.out2, got2, "makeNumericAndprioritize")
+			utils.MustMatch(t, tcase.out1, got1, "makeNumericAndprioritize")
+			utils.MustMatch(t, tcase.out2, got2, "makeNumericAndprioritize")
 		})
 	}
 }

go/vt/vtgate/plugin_mysql_server.go

@@ -218,8 +218,8 @@ func (vh *vtgateHandler) ComQuery(c *mysql.Conn, query string, callback func(*sq
 		return mysql.NewSQLErrorFromError(err)
 	}
 	session, result, err := vh.vtg.Execute(ctx, session, query, make(map[string]*querypb.BindVariable))
-	err = mysql.NewSQLErrorFromError(err)
-	if err != nil {
+
+	if err := mysql.NewSQLErrorFromError(err); err != nil {
 		return err
 	}
 	fillInTxStatusFlags(c, session)