-
Notifications
You must be signed in to change notification settings - Fork 1
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
add error_record_table #68
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,96 @@ | ||
--- | ||
feature: error_record_table | ||
authors: | ||
- "TennyZhuang" | ||
start_date: "2023/07/31" | ||
--- | ||
|
||
# Error Record Table | ||
|
||
## Summary | ||
|
||
Our current streaming engine does not help users to discover, debug, and handle errors well. When user met an data record error, they can only find a log record like ``ExprError: Parse error: expected `,` or `]` at line 1 column 10 (ProjectExecutor: fragment_id=19007)``. | ||
|
||
User can't view the eror record, and can't replay with the error record. | ||
|
||
We want to introduce the Error Record Table (ERT) to resolve the problem. | ||
|
||
## Motivation | ||
|
||
There are several benefits to maintain the error records ourselves: | ||
|
||
1. We can ensure that our storage engine can handle the volume of erroneous data, as it is of the same magnitude as the source. | ||
2. We can ensure the error records are durable. | ||
3. Users can view the error records directly over psql. | ||
4. Users can reproduce the error easily by the similar SQL. | ||
|
||
Some common errors include: | ||
|
||
1. Expression errors (division by zero, cast fail, json extraction fail) | ||
2. UDF errors (timeout, invalid records, user errors) | ||
3. Source error (paring failed) | ||
|
||
## Design | ||
|
||
### Creating | ||
|
||
The ERTs are automatically created as internal tables when an operator is created. In most cases, an operator will have n ERTs, where n corresponds to the number of inputs it has. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So how can a user get all errors happened in a MV? I think users are not supposed to under the distributed execution plan... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, I think it sounds too heavy to keep one error table for each operator. I tend to keep one single error table for the whole database. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1. This inspires me that it makes stateless operators like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Edit: Sorry I realized that scale-out still need to create new state table, and scale in need to merge state table. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have any implementation details in this RFC? For example, how will this change the interface of expression evaluation? Will we have to pass the new |
||
|
||
Connectors should also ouptut the error records to the source ERT. | ||
|
||
### Naming | ||
|
||
Same as other internal tables while suffixed by `error_{seq}`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's the scope of ERT (one ERT per exec / per fragment) |
||
|
||
### Schema | ||
|
||
The schema of ERT should have the same fields as their input, with several extra columns: | ||
|
||
1. `id bigint`: The ID can be generated by the similar method like `row_id` (vnode + local monotical ID). | ||
2. `error_reason varchar`: A human-readable error message. | ||
Comment on lines
+49
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also error level: |
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we also have a column for the sql? Or store it in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO we may store error ID instead of varchar here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then store error_reasons in a system table. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Store the mview ID. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes we can, but have to deal with backwards compat issues, in case users construct their own mapping logic using the error ids. |
||
### Modification | ||
|
||
To keep things simple, we do not permit any DML operations over the ERT. Only the `TRUNCATE TABLE` operation is permitted. | ||
|
||
### The relationship between ERT and the log system | ||
|
||
We should keep the warning entry in our log, and we can give the error record ID in the log entry. | ||
|
||
We can even give a SQL to query the error record in the log entry if it's helpful to user. | ||
|
||
## Unresolved questions | ||
|
||
Should we allow creating sink over ERT? (Similar to side_output in Flink) | ||
|
||
## Alternatives | ||
|
||
One alternative solution is to output the complete error record directly to the log system. There are some concerns: | ||
|
||
1. The data record may be too large to record, e.g. several tens of KB. | ||
2. Errors may occur continuously, causing the log system to fill up quickly. | ||
|
||
From the viewpoint of users, many of them do not consider logs as being reliable. In our default error handling approach, we do not halt the stream when encountering stream errors, which may lead users to anticipate greater reliability in those logs. | ||
|
||
## Future possibilities | ||
|
||
### Data correction | ||
|
||
ERT could potentially be used to correct data, for example, users could clean up the data within ERT and then reimport it into the source. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The record in ERT could be an intermediate result of arbitrary operators in the plan, so it might be difficult for users to recognize or understand the values (unless he is an expert of RisingWave and knows the execution details). So I'm afraid users are not always able to operate on these "discarded" records, not to mention to correct them, except for some special ones like the errors for source parsing or type casting. |
||
|
||
```sql | ||
SELECT v1, v2, error_reason FROM __rw_internal_1023_source_1134_error_1; | ||
# 10000, 0, "division by zero" | ||
CREATE TEMP TABLE fixing_1234 (v1 int, v2 int); | ||
INSERT INTO fixing_1234 ( | ||
SELECT v1, v2 FROM __rw_internal_1023_source_1134_error_1); | ||
UPDATE fixing_1234 SET (v2 = 1) WHERE v2 = 0; | ||
INSERT INTO source_table ( | ||
SELECT * FROM fixing_1234 | ||
); | ||
TRUNCATE TABLE __rw_internal_1023_source_1134_error_1; | ||
``` | ||
|
||
### Sink | ||
|
||
For advanced users, we can still allow them sink the error records to their own system. |
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.
FWIW, currently we also have error reporting to Prometheus (risingwavelabs/risingwave#7824), but it do not contains original data (seemly for the same reason described in the Alternatives section)
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.
@neverchanje proposed similar ideas in risingwavelabs/risingwave#7803
Closed as "we are now able to find the errors from Grafana", but we haven't supported "allow users to persist lost messages" yet.
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.
Yeah. According to our discussion today, I feel that the major motivation of this RFC is to provide "dead letter table" per operator, so that user can find the raw records that caused the problem.
I totally agree with the idea but the approach seems to be too difficult for a common user, especially when they faces tens of tables and they can hardly tell which table to look into.