-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
sql, pgwire: Add SEVERITY_NONLOCALIZED
error field
#82677
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code looks good! could you update the tests with
./dev test pkg/sql/pgwire --filter='TestPGTest/notice' --rewrite
also, can you make the release note be (sql change)
and mention that we now send the SeverityUnlocalized field in the pgwire NoticeResponse.
pkg/sql/pgwire/pgerror/errors.proto
Outdated
@@ -24,7 +24,8 @@ message Error { | |||
string detail = 3; | |||
string hint = 4; | |||
string severity = 8; | |||
string constraint_name = 9; | |||
string severity_nonlocalized = 9; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh interesting, didn't realize this would need to change
the important thing about proto
definitions is that the order matters. if you change the order, it changes how the object is serialized, which means code that is using the old definition won't know how to deserialize it
so, you need to make severity_nonlocalized
be at number 10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realized this was left over code I meant to remove.
Release note (sql change): We now send the Severity_Nonlocalized field in the pgwire Notice Response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
bors r+ |
Build succeeded: |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from ea6ded2 to blathers/backport-release-22.1-82677: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 22.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
Resolves #81794
Release note (sql change): We now send the Severity_Nonlocalized field
in the pgwire Notice Response.