Tracking: Eliminate usages of RwError

[fuyufjh]

@BugenZhao (Nov 29, 2023): See motivation here #13588 (comment)

---

Also I suggest avoiding using RwError everywhere. RwError is designed to have 1-1 correspondence with Postgres. However, not all our results will be reported to Postgres shell -- meta error, cli error, dashboard error, stream error, they won't have any interaction with Postgres.

I think the first step is to introduce per-crate error type.

Originally posted by @skyzh in #1367 (comment)

• streaming: eliminate usages of RwError #3116
• batch: eliminate usages of RwError #4368
• Tracking: Introduce per-module error type for frontend #2366
• meta: eliminate usages of RwError #4168
• storage: eliminate usages of RwError
• connector: eliminate usages of RwError #4105
• refactor: Replace RwError with dedicated error types in StorageTable #5272
• refactor: Replace RwError with dedicated error types in StateTable #5273

[BugenZhao]

Related:

• Tracking: Introduce per-module error type for frontend #2366

[fuyufjh]

I decided to bring this back to milestone because RwError seems to be a potential performance killer, and it's very hard to use any testing to eliminate. Refactoring seems the only way.

Batch and connector mod seems to be the major blockers now. Please take a look ❤️ cc. @tabVersion @liurenjie1024

[liurenjie1024]

I decided to bring this back to milestone because RwError seems to be a potential performance killer, and it's very hard to use any testing to eliminate. Refactoring seems the only way.

Batch and connector mod seems to be the major blockers now. Please take a look ❤️ cc. @tabVersion @liurenjie1024

Why RwError is a performance killer? Since it may use backtrace?

[fuyufjh]

Yeah, it always captures the stacktrace.

I think we need to eliminate usage of RwError completely inside RisingWave, but only use it as a way to return Error to Users. Btw, @BugenZhao is working on a more sophisticated solution, but this is a good starting point.

[liurenjie1024]

Yeah, it always captures the stacktrace.

I think we need to eliminate usage of RwError completely inside RisingWave, but only use it as a way to return Error to Users. Btw, @BugenZhao is working on a more sophisticated solution, but this is a good starting point.

Hmm, then we will lose contexts.

[BugenZhao]

Hmm, then we will lose contexts.

I guess providing contexts doesn't have to be done through backtrace. For errors triggered by users (like ExprError), the original input could be much more meaningful than a static backtrace. We can choose some of the error variants that are believed as internal errors and add the backtrace field specifically for them.

[fuyufjh]

Yeah, it always captures the stacktrace.
I think we need to eliminate usage of RwError completely inside RisingWave, but only use it as a way to return Error to Users. Btw, @BugenZhao is working on a more sophisticated solution, but this is a good starting point.

Hmm, then we will lose contexts.

As Bugen mentioned, we can keep some backtrace for internal errors. But for user-facing errors such as parser error, it doesn't make sense to capture the stacktrace

[fuyufjh]

@BugenZhao PTAL

[BugenZhao]

Only the frontend crate remains to be refactored. I'd like to close this issue and let #2366 track that.