cli: promote the 5-character error codes in the error outputs

tldr: This patch opens a new era in the relationship between
CockroachDB and its users by acknowledging the existence of SQLSTATE
in the error outputs produced by CLI commands.

Before:
```
root@127.0.0.1:61772/movr> select * from u;
pq: relation "u" does not exist
```

After:
```
root@127.0.0.1:61772/movr> select * from u;
ERROR: relation "u" does not exist
SQLSTATE: 42P01
```

Background:

The SQL standard specifies that servers should signal errors to
clients with both an explanatory text and a 5-character "SQLSTATE"
code. Certain of these codes are even standardized. The purpose of
these codes is to facilitate both troubleshooting by end-users and
easier searching for resources about what to do under certain errors.

PostgreSQL makes consistent use of these 5-digit codes; they are
always communicated to clients as a special payload in the postgres
wire protocol. In fact, CockroachDB has started ever since version 2.0
to imitate PostgreSQL and try an produce similar codes in similar
circumstances.

Unfortunately, CockroachDB's own SQL shell does not show SQLSTATE
codes to the user and this fails to teach users about their existence.

Moreover, users who build automation without knowing about SQLSTATE
codes get tempted to rely on the text of the error message
instead. This is undesirable because we wish to be able to improve
error messages (to clarify them over time) and only promise more
stability guarantees on the SQLSTATE.

This patch improve supon this situation by promoting SQLSTATE
in the default output of CLI commands.

Additional reading resources:

- https://www.postgresql.org/docs/12/errcodes-appendix.html
- https://en.wikipedia.org/wiki/SQLSTATE

Additionally, in order to prepare for the upcoming `pgx` migration,
the `pq: ` prefix to error generated by `lib/pq` is stripped out. It
is replaced by the "Severity" field of the pgwire error packet, if
available.

Release note (cli change): the various CLI commands that use SQL now
display errors using a new display format that emphasizes the 5-digit
SQLSTATE code. Users are encouraged to combine these codes together
with the error message when seeking help or troubleshooting.

pkg/cli/cli.go

@@ -71,8 +71,7 @@ func exitWithError(cmdName string, err error) {
 	errCode := 0
 	if err != nil {
 		// Display the error and its details/hints.
-		fmt.Fprintln(stderr, "Error:", err.Error())
-		maybeShowErrorDetails(stderr, err, false /* printNewline */)
+		cliOutputError(stderr, err, true /*showSeverity*/, false /*verbose*/)
 
 		// Remind the user of which command was being run.
 		fmt.Fprintf(stderr, "Failed running %q\n", cmdName)

pkg/cli/cli_test.go

@@ -406,8 +406,7 @@ func (c cliTest) runWithArgsUnredirected(origArgs []string) {
 
 		return Run(args)
 	}(); err != nil {
-		fmt.Println(err)
-		maybeShowErrorDetails(os.Stdout, err, false /*printNewLine*/)
+		cliOutputError(os.Stdout, err, true /*showSeverity*/, false /*verbose*/)
 	}
 }
 
@@ -515,7 +514,8 @@ func Example_demo() {
 	// 1
 	// 1
 	// demo --set=errexit=0 -e select nonexistent -e select 123 as "123"
-	// pq: column "nonexistent" does not exist
+	// ERROR: column "nonexistent" does not exist
+	// SQLSTATE: 42703
 	// 123
 	// 123
 	// demo startrek -e show databases
@@ -607,14 +607,15 @@ func Example_sql() {
 	// sql -d nonexistent -e create database nonexistent; create table foo(x int); select * from foo
 	// x
 	// sql -e copy t.f from stdin
-	// woops! COPY has confused this client! Suggestion: use 'psql' for COPY
+	// ERROR: woops! COPY has confused this client! Suggestion: use 'psql' for COPY
 	// user ls --echo-sql
 	// warning: This command is deprecated. Use SHOW USERS or SHOW ROLES in a SQL session.
 	// > SHOW USERS
 	// user_name
 	// root
 	// sql --set=errexit=0 -e select nonexistent -e select 123 as "123"
-	// pq: column "nonexistent" does not exist
+	// ERROR: column "nonexistent" does not exist
+	// SQLSTATE: 42703
 	// 123
 	// 123
 	// sql --set echo=true -e select 123 as "123"
@@ -623,7 +624,7 @@ func Example_sql() {
 	// 123
 	// sql --set unknownoption -e select 123 as "123"
 	// invalid syntax: \set unknownoption. Try \? for help.
-	// invalid syntax
+	// ERROR: invalid syntax
 }
 
 func Example_sql_watch() {
@@ -641,7 +642,8 @@ func Example_sql_watch() {
 	// 0.5
 	// dec
 	// 1
-	// pq: division by zero
+	// ERROR: division by zero
+	// SQLSTATE: 22012
 }
 
 func Example_sql_format() {
@@ -1125,7 +1127,8 @@ func Example_sql_table() {
 	// sql -e insert into t.t values (e'a\tb\tc\n12\t123123213\t12313', 'tabs')
 	// INSERT 1
 	// sql -e insert into t.t values (e'\xc3\x28', 'non-UTF8 string')
-	// pq: lexical error: invalid UTF-8 byte sequence
+	// ERROR: lexical error: invalid UTF-8 byte sequence
+	// SQLSTATE: 42601
 	// DETAIL: source SQL:
 	// insert into t.t values (e'\xc3\x28', 'non-UTF8 string')
 	//                         ^
@@ -1484,13 +1487,13 @@ func Example_user() {
 	// CREATE USER 1
 	// user set ,foo
 	// warning: This command is deprecated. Use CREATE USER or ALTER USER ... WITH PASSWORD ... in a SQL session.
-	// pq: username ",foo" invalid; usernames are case insensitive, must start with a letter, digit or underscore, may contain letters, digits, dashes, or underscores, and must not exceed 63 characters
+	// ERROR: username ",foo" invalid; usernames are case insensitive, must start with a letter, digit or underscore, may contain letters, digits, dashes, or underscores, and must not exceed 63 characters
 	// user set f,oo
 	// warning: This command is deprecated. Use CREATE USER or ALTER USER ... WITH PASSWORD ... in a SQL session.
-	// pq: username "f,oo" invalid; usernames are case insensitive, must start with a letter, digit or underscore, may contain letters, digits, dashes, or underscores, and must not exceed 63 characters
+	// ERROR: username "f,oo" invalid; usernames are case insensitive, must start with a letter, digit or underscore, may contain letters, digits, dashes, or underscores, and must not exceed 63 characters
 	// user set foo,
 	// warning: This command is deprecated. Use CREATE USER or ALTER USER ... WITH PASSWORD ... in a SQL session.
-	// pq: username "foo," invalid; usernames are case insensitive, must start with a letter, digit or underscore, may contain letters, digits, dashes, or underscores, and must not exceed 63 characters
+	// ERROR: username "foo," invalid; usernames are case insensitive, must start with a letter, digit or underscore, may contain letters, digits, dashes, or underscores, and must not exceed 63 characters
 	// user set 0foo
 	// warning: This command is deprecated. Use CREATE USER or ALTER USER ... WITH PASSWORD ... in a SQL session.
 	// CREATE USER 1
@@ -1505,7 +1508,7 @@ func Example_user() {
 	// CREATE USER 1
 	// user set foofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoof
 	// warning: This command is deprecated. Use CREATE USER or ALTER USER ... WITH PASSWORD ... in a SQL session.
-	// pq: username "foofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoof" invalid; usernames are case insensitive, must start with a letter, digit or underscore, may contain letters, digits, dashes, or underscores, and must not exceed 63 characters
+	// ERROR: username "foofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoof" invalid; usernames are case insensitive, must start with a letter, digit or underscore, may contain letters, digits, dashes, or underscores, and must not exceed 63 characters
 	// user set foofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoo
 	// warning: This command is deprecated. Use CREATE USER or ALTER USER ... WITH PASSWORD ... in a SQL session.
 	// CREATE USER 1
@@ -1701,7 +1704,7 @@ func Example_node() {
 	//    1
 	// (1 row)
 	// node status 10000
-	// Error: node 10000 doesn't exist
+	// ERROR: node 10000 doesn't exist
 	// sql -e drop database defaultdb
 	// DROP DATABASE
 	// node ls
@@ -1726,7 +1729,8 @@ func TestCLITimeout(t *testing.T) {
 		}
 
 		const exp = `node status 1 --all --timeout 1ns
-pq: query execution canceled due to statement timeout
+ERROR: query execution canceled due to statement timeout
+SQLSTATE: 57014
 `
 		if out != exp {
 			err := errors.Errorf("unexpected output:\n%q\nwanted:\n%q", out, exp)
@@ -2088,7 +2092,7 @@ func TestJunkPositionalArguments(t *testing.T) {
 		if err != nil {
 			t.Fatal(errors.Wrap(err, strconv.Itoa(i)))
 		}
-		exp := fmt.Sprintf("%s\nunknown command %q for \"cockroach %s\"\n", line, junk, test)
+		exp := fmt.Sprintf("%s\nERROR: unknown command %q for \"cockroach %s\"\n", line, junk, test)
 		if exp != out {
 			t.Errorf("expected:\n%s\ngot:\n%s", exp, out)
 		}
@@ -2388,7 +2392,7 @@ func Example_dump_no_visible_columns() {
 	// CREATE TABLE t (,
 	// 	FAMILY "primary" (rowid)
 	// );
-	// table "defaultdb.public.t" has no visible columns
+	// ERROR: table "defaultdb.public.t" has no visible columns
 	// HINT: To proceed with the dump, either omit this table from the list of tables to dump, drop the table, or add some visible columns.
 	// --
 	// See: https://github.com/cockroachdb/cockroach/issues/37768

pkg/cli/error.go

@@ -14,12 +14,15 @@ import (
 	"context"
 	"crypto/x509"
 	"fmt"
+	"io"
 	"net"
 	"regexp"
+	"strconv"
 	"strings"
 
 	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/util/grpcutil"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/netutil"
@@ -30,6 +33,124 @@ import (
 	"google.golang.org/grpc/status"
 )
 
+// cliOutputError prints out an error object on the given writer.
+//
+// It has a somewhat inconvenient set of requirements: it must make
+// the error both palatable to a human user, which mandates some
+// beautification, and still retain a few guarantees for automatic
+// parsers (and a modicum of care for cross-compatibility across
+// versions), including that of keeping the output relatively stable.
+//
+// As a result, future changes should be careful to properly balance
+// changes made in favor of one audience with the needs and
+// requirements of the other audience.
+func cliOutputError(w io.Writer, err error, showSeverity, verbose bool) {
+	f := formattedError{err: err, showSeverity: showSeverity, verbose: verbose}
+	fmt.Fprintln(w, f.Error())
+}
+
+type formattedError struct {
+	err                   error
+	showSeverity, verbose bool
+}
+
+// Error implements the error interface.
+func (f *formattedError) Error() string {
+	// If we're applying recursively, ignore what's there and display the original error.
+	// This happens when the shell reports an error for a second time.
+	var other *formattedError
+	if errors.As(f.err, &other) {
+		return other.Error()
+	}
+	var buf strings.Builder
+
+	// If the severity is missing, we're going to assume it's an error.
+	severity := "ERROR"
+
+	// Extract the fields.
+	var message, code, hint, detail, location string
+	if pqErr, ok := errors.UnwrapAll(f.err).(*pq.Error); ok {
+		if pqErr.Severity != "" {
+			severity = pqErr.Severity
+		}
+		message = pqErr.Message
+		code = string(pqErr.Code)
+		hint, detail = pqErr.Hint, pqErr.Detail
+		location = formatLocation(pqErr.File, pqErr.Line, pqErr.Routine)
+	} else {
+		message = f.err.Error()
+		code = pgerror.GetPGCode(f.err)
+		// Extract the standard hint and details.
+		hint = errors.FlattenHints(f.err)
+		detail = errors.FlattenDetails(f.err)
+		if file, line, fn, ok := errors.GetOneLineSource(f.err); ok {
+			location = formatLocation(file, strconv.FormatInt(int64(line), 10), fn)
+		}
+	}
+
+	// The order of the printing goes from most to less important.
+
+	if f.showSeverity && severity != "" {
+		fmt.Fprintf(&buf, "%s: ", severity)
+	}
+	fmt.Fprintln(&buf, message)
+
+	if code != "" {
+		// In contrast to `psql` we print the code even when printing
+		// non-verbosely, because we want to promote users reporting codes
+		// when interacting with support.
+		if code == pgcode.Uncategorized && !f.verbose {
+			// An exception is made for the "uncategorized" code, because we
+			// also don't want users to get the idea they can rely on XXUUU
+			// in their apps. That code is special, as we typically seek to
+			// replace it over time by something more specific.
+			//
+			// So in this case, if not printing verbosely, we don't display
+			// the code.
+		} else {
+			fmt.Fprintln(&buf, "SQLSTATE:", code)
+		}
+	}
+
+	if detail != "" {
+		fmt.Fprintln(&buf, "DETAIL:", detail)
+	}
+	if hint != "" {
+		fmt.Fprintln(&buf, "HINT:", hint)
+	}
+	if f.verbose && location != "" {
+		fmt.Fprintln(&buf, "LOCATION:", location)
+	}
+
+	// The code above is easier to read and write by stripping the
+	// extraneous newline at the end, than ensuring it's not there in
+	// the first place.
+	return strings.TrimRight(buf.String(), "\n")
+}
+
+// formatLocation spells out the error's location in a format
+// similar to psql: routine then file:num. The routine part is
+// skipped if empty.
+func formatLocation(file, line, fn string) string {
+	var res strings.Builder
+	res.WriteString(fn)
+	if file != "" || line != "" {
+		if fn != "" {
+			res.WriteString(", ")
+		}
+		if file == "" {
+			res.WriteString("<unknown>")
+		} else {
+			res.WriteString(file)
+		}
+		if line != "" {
+			res.WriteByte(':')
+			res.WriteString(line)
+		}
+	}
+	return res.String()
+}
+
 // reConnRefused is a regular expression that can be applied
 // to the details of a GRPC connection failure.
 //

pkg/cli/error_test.go

@@ -0,0 +1,101 @@
+// Copyright 2019 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package cli
+
+import (
+	"fmt"
+	"strings"
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
+	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/cockroachdb/errors"
+	"github.com/lib/pq"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestOutputError(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	errBase := errors.New("woo")
+	file, line, fn, _ := errors.GetOneLineSource(errBase)
+	refLoc := fmt.Sprintf("%s, %s:%d", fn, file, line)
+	testData := []struct {
+		err                   error
+		showSeverity, verbose bool
+		exp                   string
+	}{
+		// Check the basic with/without severity.
+		{errBase, false, false, "woo"},
+		{errBase, true, false, "ERROR: woo"},
+		{pgerror.WithCandidateCode(errBase, pgcode.Syntax), false, false, "woo\nSQLSTATE: " + pgcode.Syntax},
+		// Check the verbose output. This includes the uncategorized sqlstate.
+		{errBase, false, true, "woo\nSQLSTATE: " + pgcode.Uncategorized + "\nLOCATION: " + refLoc},
+		{errBase, true, true, "ERROR: woo\nSQLSTATE: " + pgcode.Uncategorized + "\nLOCATION: " + refLoc},
+		// Check the same over pq.Error objects.
+		{&pq.Error{Message: "woo"}, false, false, "woo"},
+		{&pq.Error{Message: "woo"}, true, false, "ERROR: woo"},
+		{&pq.Error{Message: "woo"}, false, true, "woo"},
+		{&pq.Error{Message: "woo"}, true, true, "ERROR: woo"},
+		{&pq.Error{Severity: "W", Message: "woo"}, false, false, "woo"},
+		{&pq.Error{Severity: "W", Message: "woo"}, true, false, "W: woo"},
+		// Check hint printed after message.
+		{errors.WithHint(errBase, "hello"), false, false, "woo\nHINT: hello"},
+		// Check sqlstate printed before hint, location after hint.
+		{errors.WithHint(errBase, "hello"), false, true, "woo\nSQLSTATE: " + pgcode.Uncategorized + "\nHINT: hello\nLOCATION: " + refLoc},
+		// Check detail printed after message.
+		{errors.WithDetail(errBase, "hello"), false, false, "woo\nDETAIL: hello"},
+		// Check hint/detail collection, hint printed after detail.
+		{errors.WithHint(
+			errors.WithDetail(
+				errors.WithHint(errBase, "a"),
+				"b"),
+			"c"), false, false, "woo\nDETAIL: b\nHINT: a\n--\nc"},
+		{errors.WithDetail(
+			errors.WithHint(
+				errors.WithDetail(errBase, "a"),
+				"b"),
+			"c"), false, false, "woo\nDETAIL: a\n--\nc\nHINT: b"},
+		// Check sqlstate printed before detail, location after hint.
+		{errors.WithDetail(
+			errors.WithHint(errBase, "a"), "b"),
+			false, true, "woo\nSQLSTATE: " + pgcode.Uncategorized + "\nDETAIL: b\nHINT: a\nLOCATION: " + refLoc},
+	}
+
+	for _, tc := range testData {
+		var buf strings.Builder
+		cliOutputError(&buf, tc.err, tc.showSeverity, tc.verbose)
+		assert.Equal(t, tc.exp+"\n", buf.String())
+	}
+}
+
+func TestFormatLocation(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	testData := []struct {
+		file, line, fn string
+		exp            string
+	}{
+		{"", "", "", ""},
+		{"a.b", "", "", "a.b"},
+		{"", "123", "", "<unknown>:123"},
+		{"", "", "abc", "abc"},
+		{"a.b", "", "abc", "abc, a.b"},
+		{"a.b", "123", "", "a.b:123"},
+		{"", "123", "abc", "abc, <unknown>:123"},
+	}
+
+	for _, tc := range testData {
+		r := formatLocation(tc.file, tc.line, tc.fn)
+		assert.Equal(t, tc.exp, r)
+	}
+}

pkg/cli/interactive_tests/test_client_side_checking.tcl

@@ -40,7 +40,7 @@ send "begin;\r"
 eexpect BEGIN
 
 send "select 3+;\r"
-eexpect "pq: at or near"
+eexpect "ERROR: at or near"
 eexpect "syntax error"
 eexpect root@