sql,telemetry: report built-in function name in eval failures

Prior to this patch, if any built-in function was producing a
non-pgerror error object it would be reported in telemetry as
`othererror.eval.go:3573`.

This patch fixes this by ensuring the function name is included and
the error reported as a pg "data error",
e.g. `othererror.22000.somefunction()`.

In addition, this patch extends the connExecutor error handling code
to always include a `pgerror.Error`'s `InternalCommand` in telemetry
if set, not just when the code is "internal error" or "syntax error".

Release note (sql change): CockroachDB will now include the name of
the SQL built-in function in collected statistics upon evaluation
errors.

pkg/server/updates_test.go

@@ -296,6 +296,11 @@ func TestReportUsage(t *testing.T) {
 		) {
 			t.Fatal(err)
 		}
+		if _, err := db.Exec(`SELECT crdb_internal.set_vmodule('invalid')`); !testutils.IsError(
+			err, "comma-separated list",
+		) {
+			t.Fatal(err)
+		}
 		// If the vtable ever gets supported, change to pick one that is not supported yet.
 		if _, err := db.Exec(`SELECT * FROM pg_catalog.pg_stat_wal_receiver`); !testutils.IsError(
 			err, "virtual schema table not implemented",
@@ -528,7 +533,11 @@ func TestReportUsage(t *testing.T) {
 		"unimplemented.#9148":                                10,
 		"internalerror.":                                     10,
 		"othererror.builtins.go":                             10,
+		"othererror." +
+			pgerror.CodeDataExceptionError +
+			".crdb_internal.set_vmodule()": 10,
 		"errorcodes.blah":                                    10,
+		"errorcodes." + pgerror.CodeDataExceptionError:       10,
 		"errorcodes." + pgerror.CodeInternalError:            10,
 		"errorcodes." + pgerror.CodeSyntaxError:              10,
 		"errorcodes." + pgerror.CodeFeatureNotSupportedError: 10,
@@ -665,6 +674,7 @@ func TestReportUsage(t *testing.T) {
 		`[true,false,true] SELECT _ / _`,
 		`[true,false,true] SELECT crdb_internal.force_assertion_error(_)`,
 		`[true,false,true] SELECT crdb_internal.force_error(_, $1)`,
+		`[true,false,true] SELECT crdb_internal.set_vmodule(_)`,
 		`[true,true,false] SELECT * FROM _ WHERE (_ = _) AND (_ = _)`,
 		`[true,true,false] SELECT * FROM _ WHERE (_ = length($1::STRING)) OR (_ = $2)`,
 		`[true,true,false] SELECT _ FROM _ WHERE (_ = _) AND (lower(_) = lower(_))`,
@@ -709,6 +719,7 @@ func TestReportUsage(t *testing.T) {
 			`SELECT _ / _`,
 			`SELECT crdb_internal.force_assertion_error(_)`,
 			`SELECT crdb_internal.force_error(_, $1)`,
+			`SELECT crdb_internal.set_vmodule(_)`,
 			`SET CLUSTER SETTING "server.time_until_store_dead" = _`,
 			`SET CLUSTER SETTING "diagnostics.reporting.send_crash_reports" = _`,
 			`SET application_name = _`,

pkg/sql/conn_executor.go

@@ -314,15 +314,17 @@ func (s *Server) recordError(err error) {
 	if pgErr, ok := pgerror.GetPGCause(err); ok {
 		telemetry.Count("errorcodes." + pgErr.Code)
 
-		switch pgErr.Code {
-		case pgerror.CodeFeatureNotSupportedError:
-			if feature := pgErr.InternalCommand; feature != "" {
-				telemetry.Count("unimplemented." + feature)
-			}
-		case pgerror.CodeInternalError:
-			if trace := pgErr.InternalCommand; trace != "" {
-				telemetry.Count("internalerror." + trace)
+		if details := pgErr.InternalCommand; details != "" {
+			var prefix string
+			switch pgErr.Code {
+			case pgerror.CodeFeatureNotSupportedError:
+				prefix = "unimplemented."
+			case pgerror.CodeInternalError:
+				prefix = "internalerror."
+			default:
+				prefix = "othererror." + pgErr.Code + "."
 			}
+			telemetry.Count(prefix + details)
 		}
 	} else {
 		typ := log.ErrorSource(err)

pkg/sql/pgwire/pgerror/errors.go

@@ -23,6 +23,7 @@ import (
 	"github.com/lib/pq"
 
 	"github.com/cockroachdb/cockroach/pkg/util/caller"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/pkg/errors"
 )
 
@@ -224,3 +225,38 @@ func UnimplementedWithDepth(depth int, feature, msg string, args ...interface{})
 	err.Hint = unimplementedErrorHint
 	return err
 }
+
+// Wrap wraps an error into a pgerror. The code is used
+// if the original error was not a pgerror already. The errContext
+// string is used to populate the InternalCommand. If InternalCommand
+// already exists, the errContext is prepended.
+func Wrap(err error, code, errContext string) error {
+	var pgErr Error
+	origErr, ok := GetPGCause(err)
+	if ok {
+		// Copy the error. We can't use the existing error directly
+		// because it may be a global (const) object and we want to modify
+		// it below.
+		pgErr = *origErr
+	} else {
+		pgErr = Error{
+			Code: code,
+			// Keep the stack trace if one was available in the original
+			// non-Error error (e.g. when constructed via errors.Wrap).
+			InternalCommand: log.ErrorSource(err),
+		}
+	}
+
+	// Prepend the context to the existing message.
+	prefix := errContext + ": "
+	pgErr.Message = prefix + err.Error()
+
+	// Prepend the context also to the internal command, to ensure it
+	// goes to telemetry.
+	if pgErr.InternalCommand != "" {
+		pgErr.InternalCommand = prefix + pgErr.InternalCommand
+	} else {
+		pgErr.InternalCommand = errContext
+	}
+	return &pgErr
+}

pkg/sql/sem/tree/eval.go

@@ -3418,7 +3418,7 @@ func (expr *FuncExpr) Eval(ctx *EvalContext) (Datum, error) {
 		if fName == `crdb_internal.force_error` {
 			return nil, err
 		}
-		return nil, errors.Wrapf(err, "%s()", fName)
+		return nil, pgerror.Wrap(err, pgerror.CodeDataExceptionError, fName+"()")
 	}
 	if ctx.TestingKnobs.AssertFuncExprReturnTypes {
 		if err := ensureExpectedType(expr.fn.FixedReturnType(), res); err != nil {