cockroachdb · knz · Mar 21, 2019 · Mar 25, 2019
@@ -802,17 +802,14 @@ func isRetryableSinkError(err error) bool {
 		if _, ok := err.(*retryableSinkError); ok {
 			return true
 		}
-		// TODO(mrtracy): This pathway, which occurs when the retryable error is
-		// detected on a non-local node of the distsql flow, is only currently
-		// being tested with a roachtest, which is expensive. See if it can be
-		// tested via a unit test.
-		if _, ok := err.(*pgerror.Error); ok {
-			return strings.Contains(err.Error(), retryableSinkErrorString)
-		}
 		if e, ok := err.(causer); ok {
 			err = e.Cause()
 			continue
 		}
-		return false
+		// TODO(mrtracy): This pathway, which occurs when the retryable error is
+		// detected on a non-local node of the distsql flow, is only currently
+		// being tested with a roachtest, which is expensive. See if it can be
+		// tested via a unit test.
+		return strings.Contains(err.Error(), retryableSinkErrorString)
 	}
 }
diff --git a/pkg/ccl/importccl/read_import_pgdump.go b/pkg/ccl/importccl/read_import_pgdump.go
@@ -288,9 +288,6 @@ func readPostgresCreateTable(
 			return ret, nil
 		}
 		if err != nil {
-			if pg, ok := pgerror.GetPGCause(err); ok {
-				return nil, errors.Errorf("%s\n%s", pg.Message, pg.Detail)
-			}
 			return nil, errors.Wrap(err, "postgres parse error")
 		}
 		switch stmt := stmt.(type) {

@@ -1484,7 +1484,7 @@ func (c *cliState) serverSideParse(sql string) (stmts []string, pgErr *pgerror.E
 		// detected by SHOW SYNTAX (those show up as valid rows) but
 		// instead something else. Do our best to convert that something
 		// else back to a pgerror.Error.
-		if pgErr, ok := err.(*pgerror.Error); ok {
+		if pgErr, ok := pgerror.GetPGCause(err); ok {
 			return nil, pgErr
 		} else if pqErr, ok := err.(*pq.Error); ok {
 			return nil, pgerror.NewError(

@@ -129,7 +129,7 @@ func (s *adminServer) getUser(_ protoutil.Message) string {
 // serverError logs the provided error and returns an error that should be returned by
 // the RPC endpoint method.
 func (s *adminServer) serverError(err error) error {
-	log.ErrorfDepth(context.TODO(), 1, "%s", err)
+	log.ErrorfDepth(context.TODO(), 1, "%+v", err)
 	return errAdminAPIError
 }
 

@@ -18,8 +18,6 @@ import (
 	"fmt"
 	"sync/atomic"
 
-	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
-	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/metric"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 )
@@ -195,36 +193,3 @@ func GetAndResetFeatureCounts(quantize bool) map[string]int32 {
 	}
 	return m
 }
-
-// RecordError takes an error and increments the corresponding count
-// for its error code, and, if it is an unimplemented or internal
-// error, the count for that feature or the internal error's shortened
-// stack trace.
-func RecordError(err error) {
-	if err == nil {
-		return
-	}
-
-	if pgErr, ok := pgerror.GetPGCause(err); ok {
-		Count("errorcodes." + pgErr.Code)
-
-		if details := pgErr.TelemetryKey; details != "" {
-			var prefix string
-			switch pgErr.Code {
-			case pgerror.CodeFeatureNotSupportedError:
-				prefix = "unimplemented."
-			case pgerror.CodeInternalError:
-				prefix = "internalerror."
-			default:
-				prefix = "othererror." + pgErr.Code + "."
-			}
-			Count(prefix + details)
-		}
-	} else {
-		typ := log.ErrorSource(err)
-		if typ == "" {
-			typ = "unknown"
-		}
-		Count("othererror." + typ)
-	}
-}
diff --git a/pkg/server/updates_test.go b/pkg/server/updates_test.go
@@ -591,6 +591,10 @@ func TestReportUsage(t *testing.T) {
 	}
 	for key, expected := range expectedFeatureUsage {
 		if got, ok := r.last.FeatureUsage[key]; !ok {
+			t.Log("feature usage:")
+			for k, v := range r.last.FeatureUsage {
+				t.Logf("%s: %v", k, v)
+			}
 			t.Fatalf("expected report of feature %q", key)
 		} else if got != expected {
 			t.Fatalf("expected reported value of feature %q to be %d not %d", key, expected, got)

diff --git a/pkg/sql/conn_executor.go b/pkg/sql/conn_executor.go
@@ -37,7 +37,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
-	"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlutil"
 	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/envutil"
@@ -1336,7 +1335,6 @@ func (ex *connExecutor) run(
 				// to call either Close or CloseWithErr.
 				res.CloseWithErr(pe.errorCause())
 			} else {
-				ex.recordError(resErr)
 				res.Close(stateToTxnStatusIndicator(ex.machine.CurState()))
 			}
 		} else {
@@ -2045,13 +2043,6 @@ func (ex *connExecutor) initStatementResult(
 	return nil
 }
 
-// recordError processes an error at the end of query execution.
-// This triggers telemetry and, if the error is an internal error,
-// triggers the emission of a sentry report.
-func (ex *connExecutor) recordError(err error) {
-	sqltelemetry.RecordError(ex.Ctx(), err, &ex.server.cfg.Settings.SV)
-}
-
 // newStatsCollector returns an sqlStatsCollector that will record stats in the
 // session's stats containers.
 func (ex *connExecutor) newStatsCollector() sqlStatsCollector {

diff --git a/pkg/sql/conn_executor_exec.go b/pkg/sql/conn_executor_exec.go
@@ -31,6 +31,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/types"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
 	"github.com/cockroachdb/cockroach/pkg/util/fsm"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -840,17 +841,12 @@ func enhanceErrWithCorrelation(err error, isCorrelated bool) error {
 	// not supported") because perhaps there was an actual mistake in
 	// the query in addition to the unsupported correlation, and we also
 	// want to give a chance to the user to fix mistakes.
-	if pgErr, ok := pgerror.GetPGCause(err); ok {
-		if pgErr.Code == pgerror.CodeUndefinedColumnError ||
-			pgErr.Code == pgerror.CodeUndefinedTableError {
-			// Be careful to not modify the error in-place (via SetHintf) as
-			// the error object may be globally instantiated.
-			newErr := *pgErr
-			pgErr = &newErr
-			_ = pgErr.SetHintf("some correlated subqueries are not supported yet - see %s",
-				"https://github.com/cockroachdb/cockroach/issues/3288")
-			return pgErr
-		}
+	if code := pgerror.GetCode(err, ""); code == pgerror.CodeUndefinedColumnError ||
+		code == pgerror.CodeUndefinedTableError {
+		// Be careful to not modify the error in-place (via SetHintf) as
+		// the error object may be globally instantiated.
+		return pgerror.WithHintf(err, "some correlated subqueries are not supported yet - see %s",
+			"https://github.com/cockroachdb/cockroach/issues/3288")
 	}
 	return err
 }
@@ -1038,8 +1034,7 @@ func (ex *connExecutor) saveLogicalPlanDescription(
 // canFallbackFromOpt returns whether we can fallback on the heuristic planner
 // when the optimizer hits an error.
 func canFallbackFromOpt(err error, optMode sessiondata.OptimizerMode, stmt *Statement) bool {
-	pgerr, ok := pgerror.GetPGCause(err)
-	if !ok || pgerr.Code != pgerror.CodeFeatureNotSupportedError {
+	if code := pgerror.GetCode(err, ""); code != pgerror.CodeFeatureNotSupportedError {
 		// We only fallback on "feature not supported" errors.
 		return false
 	}
@@ -1375,7 +1370,14 @@ func (ex *connExecutor) runShowSyntax(
 			commErr = res.AddRow(ctx, tree.Datums{tree.NewDString(field), tree.NewDString(msg)})
 			return nil
 		},
-		ex.recordError, /* reportErr */
+		func(err error) {
+			// SHOW SYNTAX translates most errors into a value sent back to
+			// the client. To ensure proper reporting, we need to do reporting
+			// work here as the error will not flow back through the regular
+			// error tests.
+			pgErr, safeDetails, keys := pgerror.Flatten(err)
+			sqltelemetry.RecordError(ctx, &ex.server.cfg.Settings.SV, pgErr, safeDetails, keys)
+		},
 	); err != nil {
 		res.SetError(err)
 	}

diff --git a/pkg/sql/conn_executor_prepare.go b/pkg/sql/conn_executor_prepare.go
@@ -345,12 +345,8 @@ func (ex *connExecutor) execBind(
 			} else {
 				d, err := pgwirebase.DecodeOidDatum(ptCtx, t, qArgFormatCodes[i], arg)
 				if err != nil {
-					if _, ok := pgerror.GetPGCause(err); ok {
-						return retErr(err)
-					}
 					return retErr(pgerror.Wrapf(err, pgerror.CodeProtocolViolationError,
 						"error in argument for %s", k))
-
 				}
 				qargs[k] = d
 			}

@@ -40,6 +40,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/types"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
 	"github.com/cockroachdb/cockroach/pkg/storage/storagepb"
 	"github.com/cockroachdb/cockroach/pkg/util/json"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -880,7 +881,7 @@ var crdbInternalLocalSessionsTable = virtualSchemaTable{
 		if err != nil {
 			return err
 		}
-		return populateSessionsTable(ctx, addRow, response)
+		return p.populateSessionsTable(ctx, addRow, response)
 	},
 }
 
@@ -895,11 +896,11 @@ var crdbInternalClusterSessionsTable = virtualSchemaTable{
 		if err != nil {
 			return err
 		}
-		return populateSessionsTable(ctx, addRow, response)
+		return p.populateSessionsTable(ctx, addRow, response)
 	},
 }
 
-func populateSessionsTable(
+func (p *planner) populateSessionsTable(
 	ctx context.Context, addRow func(...tree.Datum) error, response *serverpb.ListSessionsResponse,
 ) error {
 	for _, session := range response.Sessions {
@@ -939,13 +940,11 @@ func populateSessionsTable(
 			// TODO(knz): NewInternalTrackingError is misdesigned. Change to
 			// not use this. See the other facilities in
 			// pgerror/internal_errors.go.
-			telemetry.RecordError(
-				pgerror.NewInternalTrackingError(32517 /* issue */, "null"))
+			sqltelemetry.RecordInternalTrackingError(ctx, &p.ExecCfg().Settings.SV, 32517 /* issue */, "null")
 			sessionID = tree.DNull
 		} else if len(session.ID) != 16 {
 			// TODO(knz): ditto above.
-			telemetry.RecordError(
-				pgerror.NewInternalTrackingError(32517 /* issue */, fmt.Sprintf("len=%d", len(session.ID))))
+			sqltelemetry.RecordInternalTrackingError(ctx, &p.ExecCfg().Settings.SV, 32517 /* issue */, fmt.Sprintf("len=%d", len(session.ID)))
 			sessionID = tree.NewDString("<invalid>")
 		} else {
 			clusterSessionID := BytesToClusterWideID(session.ID)

diff --git a/pkg/sql/distsqlpb/data.go b/pkg/sql/distsqlpb/data.go
@@ -21,6 +21,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqlerror"
 	"github.com/cockroachdb/cockroach/pkg/util/encoding"
 	"github.com/pkg/errors"
 )
@@ -138,39 +139,42 @@ func (e *Error) String() string {
 // recognize certain errors and marshall them accordingly, and everything
 // unrecognized is turned into a PGError with code "internal".
 func NewError(err error) *Error {
-	// Unwrap the error, to attain the cause.
-	// Otherwise, Wrap() may hide the roachpb error
-	// from the cast attempt below.
-	origErr := err
-	err = errors.Cause(err)
-
-	if pgErr, ok := pgerror.GetPGCause(err); ok {
-		return &Error{Detail: &Error_PGError{PGError: pgErr}}
-	} else if retryErr, ok := err.(*roachpb.UnhandledRetryableError); ok {
-		return &Error{
-			Detail: &Error_RetryableTxnError{
-				RetryableTxnError: retryErr,
-			}}
+	resErr := &Error{}
+
+	// Encode the full error to the best of our ability.
+	// This field is ignored by pre-19.1 nodes.
+	fullError := sqlerror.EncodeError(err)
+	resErr.FullError = &fullError
+
+	// Now populate compatibility fields for pre-19.1 nodes.
+	if retryErr, ok := errors.Cause(err).(*roachpb.UnhandledRetryableError); ok {
+		resErr.Detail = &Error_RetryableTxnError{RetryableTxnError: retryErr}
 	} else {
-		// Anything unrecognized is an "internal error".
-		return &Error{
-			Detail: &Error_PGError{
-				PGError: pgerror.NewAssertionErrorf(
-					"uncaught error: %+v", origErr)}}
+		pgErr, _, _ := pgerror.Flatten(err)
+		resErr.Detail = &Error_PGError{PGError: pgErr}
 	}
+	return resErr
 }
 
 // ErrorDetail returns the payload as a Go error.
 func (e *Error) ErrorDetail() error {
 	if e == nil {
 		return nil
 	}
+
+	if e.FullError != nil {
+		// If there's a 19.1-forward full error, decode and use that.
+		// This will reveal a fully causable detailed error structure.
+		return e.FullError.GetError()
+	}
+
+	// Fallback to pre-19.1 logic.
 	switch t := e.Detail.(type) {
 	case *Error_PGError:
 		return t.PGError
 	case *Error_RetryableTxnError:
 		return t.RetryableTxnError
 	default:
-		panic(fmt.Sprintf("bad error detail: %+v", t))
+		return pgerror.NewAssertionErrorf("bad error detail: %+v", t)
 	}
 }