sqlsmith bug hunt: channels not propagating errors

[kwannoel]

Some expressions might only fail during runtime, for instance if they only fetch columns.
Errors should be propagated. In the following cases, the underlying error is NumericOutOfRangeError, but it is not propagated to downstream executors.

dev=> create table t(v int);                                                                                                                                                                                                    CREATE_TABLE
dev=> insert into t values (33);                                                                                                                                                                                                             INSERT 0 1
dev=> select (SMALLINT '22' << v) from t;                                                                                                                                                                                                    ERROR:  RPC error: Status { code: Internal, message: "internal error: broken fifo_channel", metadata: MetadataMap { headers: {"risingwave-error-bin": "CAESI2ludGVybmFsIGVycm9yOiBicm9rZW4gZmlmb19jaGFubmVs"} }, source: None }

dev=> create table t(v int);                                                                                                                                                                                                    CREATE_TABLE
dev=> insert into t values (33);                                                                                                                                                                                                             INSERT 0 1
dev=> select (SMALLINT '22' << v) from t;                                                                                                                                                                                                    ERROR:  RPC error: Status { code: Internal, message: "internal error: broken fifo_channel", metadata: MetadataMap { headers: {"risingwave-error-bin": "CAESI2ludGVybmFsIGVycm9yOiBicm9rZW4gZmlmb19jaGFubmVs"} }, source: None }
dev => insert into t values(0);
INSERT 0 1
dev=> select (SMALLINT '33' / SMALLINT v) from t;
ERROR:  RPC error: Status { code: Internal, message: "internal error: broken fifo_channel", metadata: MetadataMap { headers: {"risingwave-error-bin": "CAESI2ludGVybmFsIGVycm9yOiBicm9rZW4gZmlmb19jaGFubmVs"} }, source: None }

[kwannoel]

This is because the channel we use between executors passes Optionals around:
https://github.com/singularity-data/risingwave/blob/b64d8294c091413fa517eb70a0659ae3c6bcd3d0/src/batch/src/task/fifo_channel.rs#L31

And so when we read from the channel, the errors are not propagated:
https://github.com/singularity-data/risingwave/blob/b64d8294c091413fa517eb70a0659ae3c6bcd3d0/src/batch/src/task/fifo_channel.rs#L47-L59

Am thinking of changing the Item type in fifo channel to Result for error propagation. Any other suggestions?

[skyzh]

Result might not be serializable and cannot be sent over the network. I think the current implementation is okay. If there's an error, we can find it on upstream compute node?

We can modify the error message to be more user friendly, like "[E2333] internal error when computing", where E2333 = fifo channel broken. Then developers can go into the log dashboard (maybe ELK in the future) to find what's happening.

[kwannoel]

Result might not be serializable and cannot be sent over the network. I think the current implementation is okay. If there's an error, we can find it on upstream compute node?

We can modify the error message to be more user friendly, like "[E2333] internal error when computing", where E2333 = fifo channel broken. Then developers can go into the log dashboard (maybe ELK in the future) to find what's happening.

That makes sense, the above behaviour should be undefined, following PostgreSQL, so it is alright to return internal error.

Instead our fuzzers need to generate queries w/o undefined behaviour, since those may raise internal errors as above (which we can't distinguish with actual internal system errors).

[kwannoel]

Actually what about other defined errors? E.g. division by zero. These errors are also not propagated in RisingWave but in postgres they return the relevant error code. 🤔

RisingWave

dev=> create table t(v int);                                                                                                                                                                                                    CREATE_TABLE
dev => insert into t values(0);
INSERT 0 1
dev=> select (SMALLINT '33' / SMALLINT v) from t;
ERROR:  RPC error: Status { code: Internal, message: "internal error: broken fifo_channel", metadata: MetadataMap { headers: {"risingwave-error-bin": "CAESI2ludGVybmFsIGVycm9yOiBicm9rZW4gZmlmb19jaGFubmVs"} }, source: None }

Postgres should throw division by zero error:

postgres=# create table t(v int);
CREATE TABLE
postgres=# insert into t values(0);
INSERT 0 1
postgres=# select (1/v) from t;
ERROR:  division by zero

Related: #3939

[Graphcalibur]

Related issue: #2473

[kwannoel]

Had an offline discussion with @lmatz and he suggested the following:

Multiple upstream executors may yield multiple errors, causing the errors to accumulate. Does not make sense to pass all these downstream.
One way to deal with this is by having a separate mechanism for tracking all errors.
We can push all errors to some error queue as they are encountered, then frontend can use this to view errors.

This currently blocks sqlsmith work, since we can't know what the source of broken fifo channel error is: Runtime error? System internal error? etc... This means we won't know if a query is failing due to runtime error or system internal error.

This issue will take a fair amount of work to resolve.
As a workaround to unblock SqlSmith work, we let broken fifo channel errors pass through for now.

[jon-chuang]

For stream, we can collect the actor errors here: https://github.com/singularity-data/risingwave/blob/b64d8294c091413fa517eb70a0659ae3c6bcd3d0/src/stream/src/task/stream_manager.rs#L621 and then pipe it directly from the node to the frontend.

[xxchan]

related issue #3730

[BowenXiao1999]

Create a issue relates to this: #4811