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.

Sign up for GitHub

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

feat(error): report the error's source chain through psql #13264

Merged
merged 15 commits into from
Nov 9, 2023
+111 −46
Merged

feat(error): report the error's source chain through psql #13264

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
589d525
as report
BugenZhao Nov 6, 2023
a474acb
gate on pretty error
BugenZhao Nov 6, 2023
148b368
refine panic error message
BugenZhao Nov 7, 2023
0a887c0
remove cleaned-up note
BugenZhao Nov 7, 2023
8759113
always use pretty
BugenZhao Nov 7, 2023
723aa2d
remove prompt in sqllogictest
BugenZhao Nov 7, 2023
e8f0365
cleanup imports
BugenZhao Nov 7, 2023
e0d4500
Merge remote-tracking branch 'origin/main' into bz/display-error-source
BugenZhao Nov 7, 2023
39493a0
add ui test file
BugenZhao Nov 8, 2023
3cd7a6f
add readme
BugenZhao Nov 8, 2023
d60b7e1
add readme and bump version in risedev
BugenZhao Nov 8, 2023
2600f30
Merge remote-tracking branch 'origin/main' into bz/display-error-source
BugenZhao Nov 8, 2023
11b2834
add ui test to ci script
BugenZhao Nov 8, 2023
bb0ce45
bump ci image for sqllogictest 0.18
BugenZhao Nov 9, 2023
41c88a5
fix duplicated error message
BugenZhao Nov 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
gate on pretty error 
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@BugenZhao
BugenZhao committed Nov 7, 2023

Verified

This commit was signed with the committer’s verified signature.
KyleFromNVIDIA Kyle Edwards
SSH Key Fingerprint: HCOKmVHDGRyoBbxy3oHe2oM/H/VI74oAiMzA3TKFtmM
Verified
Learn about vigilant mode
commit a474acbc5d9a1ff33cfb6b9d844e21c06a7bf756
7 changes: 6 additions & 1 deletion src/utils/pgwire/src/pg_message.rs
View file
Original file line number Diff line number Diff line change
@@ -19,6 +19,7 @@ use std::io::{Error, ErrorKind, IoSlice, Result, Write};
use byteorder::{BigEndian, ByteOrder};
/// Part of code learned from <https://github.com/zenithdb/zenith/blob/main/zenith_utils/src/pq_proto.rs>.
use bytes::{Buf, BufMut, Bytes, BytesMut};
use risingwave_common::util::env_var::env_var_is_true;
use tokio::io::{AsyncRead, AsyncReadExt};

use crate::error_or_notice::ErrorOrNoticeMessage;
@@ -634,7 +635,11 @@ impl<'a> BeMessage<'a> {

// 'E' signalizes ErrorResponse messages
buf.put_u8(b'E');
let msg = format!("{:#}", error.as_ref().as_report());
let msg = if env_var_is_true("RW_PRETTY_ERROR") {
format!("{:#}", error.as_ref().as_report())
} else {
format!("{}", error.as_ref().as_report())
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we ever want the Compact behavior? I prefer not to offer too many choices unless there's a good reason.

Copy link
Member

@xxchan xxchan Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides, can we add a test (maybe expect test) for an error message to demonstrate the error format?

Copy link
Member Author

@BugenZhao BugenZhao Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we ever want the Compact behavior?

This could be a temporary workaround. 😕 SQL logic tests and planner tests are heavily relying on the error message, and I don't want to mess them into this PR.

Also, I'm not sure if the developers writing SQL logic tests are intended to match the full messages (instead of the interesting part only), like below. If so, then Pretty will be less friendly.

risingwave/e2e_test/source/basic/ddl.slt

Line 21 in 761130c

statement error internal error: invalid digit found in string

risingwave/e2e_test/ddl/invalid_operation.slt

Line 267 in 761130c

statement error QueryError: Bind error: update modifying the PK column is unsupported

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides, can we add a test (maybe expect test) for an error message to demonstrate the error format?

There's one in thiserror-ext. Do you want another one for RisingWave's real cases? Then I'll write an integration test connecting to the pgwire server.

Copy link
Member

@xxchan xxchan Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a temporary workaround. 😕 SQL logic tests and planner tests are heavily relying on the error message, and I don't want to mess them into this PR.

If DX is the only concern or trouble, I think we should try to fix it now or later. At least pretty should be the default behavior. (Otherwise what's the meaning of these work? lol)

I think it's ok to update tests in this PR, as the core changes are very small.

If you don't want to do it now, maybe we should introduce RW_COMPACT_ERROR, instead of RW_PRETTY_ERROR.

Do you want another one for RisingWave's real cases?

Yes, that's preferred. So that we can prevent either risingwave or thiserror-ext's unexpected changes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update error messages in slt and pretty is always enabled now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want another one for RisingWave's real cases?

Yes, that's preferred. So that we can prevent either risingwave or thiserror-ext's unexpected changes.

I find the error message is not good enough... Maybe let's do this in future PRs. 🤡

Copy link
Member

@xxchan xxchan Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's OK if the error message is not good. Just have a expect-test to let us know what's the current status is enough. 🤣 And we can see more easily how it's improved in future PRs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find writing tests with madsim is the easiest way, but the udf example does not work there. 🤡

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤡

write_err_or_notice(buf, &ErrorOrNoticeMessage::internal_error(&msg))?;
}