Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Dec 2, 2019

Conversation

knz
Copy link
Contributor

@knz knz commented Nov 26, 2019

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:

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.

@knz knz requested a review from maddyblue November 26, 2019 12:59
@knz knz requested a review from a team as a code owner November 26, 2019 12:59
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz
Copy link
Contributor Author

knz commented Nov 26, 2019

cc @rmloveland this is what we discussed last week over your schema change PR.

@knz knz force-pushed the 20191126-cli-errs branch 2 times, most recently from cff16b4 to 0666e5d Compare November 26, 2019 13:42
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.
@jordanlewis
Copy link
Member

I question whether this is the right thing by default. As you know, in psql, this is hidden behind a setting.

I already find our output more verbose then necessary (timings on by default) - do we want this on by default as well? I suppose it's just for errors, so why not?

@knz
Copy link
Contributor Author

knz commented Nov 26, 2019

I question whether this is the right thing by default. As you know, in psql, this is hidden behind a setting.

Yeah I'm on the fence about this. OTOH I really want users to start googling SQLSTATE codes more than the error texts, that will make their life easier. Not so easy if it's not printed by default.

As another data point, the error objects produced by pgx also always display SQLSTATE when you print out the error.

I already find our output more verbose then necessary (timings on by default)

For timings this was a feature ask, which did receive positive feedback. Do you think we could tweak it? I recall the need was to display how two "perceptually fast" things are not equally fast, and invite the user to think about indexes early. Do you think we can achieve this in a different way?

do we want this on by default as well? I suppose it's just for errors, so why not?

Yeah that's what I'm thinking about as well.

@jordanlewis
Copy link
Member

Okay, I take it back - I think since it's on error only, I'm okay with it. Maybe we need to dress it up a little bit though. SQLSTATE is meaningless to most people, right?

@knz
Copy link
Contributor Author

knz commented Nov 26, 2019

SQLSTATE is meaningless to most people, right?

It's meaningless to many engineers on our team but that's really a blindspot particular to CRL. The amount and quality of help you get on internet search engines when you use "SQLSTATE" and a code in your query is amazing. SQL users probably know a great deal more about it than we do.

@rmloveland
Copy link
Collaborator

FWIW I love this change - at least on macOS, I can quickly:

  1. Highlight "SQLSTATE: 42P01" in the terminal
  2. right click, choose "search with Google" from the menu
  3. see lots and lots of docs and StackOverflow posts

@knz
Copy link
Contributor Author

knz commented Dec 2, 2019

TFYR!

bors r+

craig bot pushed a commit that referenced this pull request Dec 2, 2019
42779: cli: promote the 5-character error codes in the error outputs r=knz a=knz

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.

Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
@craig
Copy link
Contributor

craig bot commented Dec 2, 2019

Build succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants