From 00ca6cb8b438533514ec6958f6804683f0223aed Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Sun, 21 Oct 2018 11:22:38 +0200 Subject: [PATCH] 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 | 11 ++++++++++ pkg/sql/conn_executor.go | 18 +++++++++------- pkg/sql/pgwire/pgerror/errors.go | 36 ++++++++++++++++++++++++++++++++ pkg/sql/sem/tree/eval.go | 2 +- 4 files changed, 58 insertions(+), 9 deletions(-) diff --git a/pkg/server/updates_test.go b/pkg/server/updates_test.go index 4bb2b15fb643..44bb7ef6920e 100644 --- a/pkg/server/updates_test.go +++ b/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 = _`, diff --git a/pkg/sql/conn_executor.go b/pkg/sql/conn_executor.go index 95516964d4e1..cd98a869beb9 100644 --- a/pkg/sql/conn_executor.go +++ b/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) diff --git a/pkg/sql/pgwire/pgerror/errors.go b/pkg/sql/pgwire/pgerror/errors.go index 6c5c82b66f1a..a62ef8a1c0c9 100644 --- a/pkg/sql/pgwire/pgerror/errors.go +++ b/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 +} diff --git a/pkg/sql/sem/tree/eval.go b/pkg/sql/sem/tree/eval.go index 0053192fb30d..6f44cfa8d4d1 100644 --- a/pkg/sql/sem/tree/eval.go +++ b/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 {