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

add error_record_table #68

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
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
96 changes: 96 additions & 0 deletions rfcs/0068-error-record-table.md
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)``.
Copy link
Member

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)

Copy link
Member

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

But when it comes to production deployment, we will need to provide an option that allows users to persist lost messages somewhere (maybe in Hummock or a log store) so that they can search for what reason and what data were lost (maybe through ElasticSearch).

Closed as "we are now able to find the errors from Grafana", but we haven't supported "allow users to persist lost messages" yet.

Copy link
Member

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.


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.
Copy link
Member

@fuyufjh fuyufjh Aug 2, 2023

Choose a reason for hiding this comment

The 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...

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

@BugenZhao BugenZhao Aug 2, 2023

Choose a reason for hiding this comment

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

+1. This inspires me that it makes stateless operators like Project and Filter contain state tables as well. We even have to handle stuff related to distributed execution for them, like scaling or plan evolution. From this perspective, it seems too invasive to me.

Copy link
Contributor

@kwannoel kwannoel Aug 3, 2023

Choose a reason for hiding this comment

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

+1. This inspires me that it makes stateless operators like Project and Filter contain state tables as well. We even have to handle stuff related to distributed execution for them, like scaling or plan evolution. From this perspective, it seems too invasive to me.

For scaling I think it should be less of a problem if we follow's Eric's approach of uuid + singleton distribution.

Edit: Sorry I realized that scale-out still need to create new state table, and scale in need to merge state table.

Copy link
Member

Choose a reason for hiding this comment

The 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 StateTable for error records everywhere?


Connectors should also ouptut the error records to the source ERT.

### Naming

Same as other internal tables while suffixed by `error_{seq}`.
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

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

also error level: warn / error / critical


Copy link
Contributor

@kwannoel kwannoel Aug 2, 2023

Choose a reason for hiding this comment

The 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 error_reason col?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we may store error ID instead of varchar here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Then store error_reasons in a system table.

Copy link
Contributor

@kwannoel kwannoel Aug 2, 2023

Choose a reason for hiding this comment

The 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 error_reason col?

Store the mview ID.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we may store error ID instead of varchar here?

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.
Copy link
Member

Choose a reason for hiding this comment

The 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.