copr: do not hide original error for MPP query that will not faillback to TiKV

[windtalker]

What problem does this PR solve?

Issue Number: close #xxx

Problem Summary:
After #23078, in order to trigger fallback, when MPP query hit some error, the original error maybe hiden and instead just return a unified error named TiFlash server timeout, so in most of the case, when MPP query fails, user will get TiFlash server timeout, this is very confusing.

What is changed and how it works?

Only hide the original error message for mpp query if this query is allowed to fallback.

Check List

Tests

• Unit test
• Integration test
• Manual test (add detailed scripts or steps below)
• No code

Side effects

• Performance regression: Consumes more CPU
• Performance regression: Consumes more Memory
• Breaking backward compatibility

Documentation

• Affects user behaviors
• Contains syntax changes
• Contains variable changes
• Contains experimental features
• Changes MySQL compatibility

Release note

Return original error message for mpp query if that query is not allowed to fallback to TiKV.

[ti-chi-bot]

[REVIEW NOTIFICATION]

This pull request has been approved by:

• LittleFall
• fzhedu

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

[sre-bot]

Please follow PR Title Format:

• pkg [, pkg2, pkg3]: what is changed

Or if the count of mainly changed packages are more than 3, use

• *: what is changed

After you have format title, you can leave a comment /run-check_title to recheck it

[zhouqiang-cl]

/run-check_title

[LittleFall]

This PR will eliminate the origin error message.

---

When fallback takes effect, error from tiflash will be recorded in the warnings.

But when we want to support other error types besides ErrTiFlashServerTimeout, we need another way to record warnings.

cc @windtalker @xuyifangreeneyes

[xuyifangreeneyes]

@LittleFall Why will the pr eliminate the origin error message?(or do you mean that #22459 will eliminate the origin error message). If fallback doesn't take effect, then the user receives the origin error message. If fallback takes effect, tidb server logs the original error and the user receives TiFlash server timeout warning.

Besides, when the query is allowed to fall back, maybe adding the original error in the warnings is better than adding the unified error TiFlash server timeout.

[windtalker]

@LittleFall Why will the pr eliminate the origin error message?(or do you mean that #22459 will eliminate the origin error message). If fallback doesn't take effect, then the user receives the origin error message. If fallback takes effect, tidb server logs the original error and the user receives TiFlash server timeout warning.

Besides, when the query is allowed to fall back, maybe adding the original error in the warnings is better than adding the unified error TiFlash server timeout.

My mistake, it was #23078 that hide the original error to make fallback work more widely.

[LittleFall]

@LittleFall Why will the pr eliminate the origin error message?(or do you mean that #22459 will eliminate the origin error message). If fallback doesn't take effect, then the user receives the origin error message. If fallback takes effect, tidb server logs the original error and the user receives TiFlash server timeout warning.

Besides, when the query is allowed to fall back, maybe adding the original error in the warnings is better than adding the unified error TiFlash server timeout.

You are right, we should not cover up the original error with tiflash server timeout. This is what 23078 does, not this pr.

I want to explain if we want to change the behavior back (throw the original error, instead of tiflash server timeout) in this pr, Do you have any suggestions about the implementation?

[fzhedu]

what about this case?
https://github.com/pingcap/tidb/pull/23078/files#diff-dec6808c7659579faf2c9c6f9d8b7a7e0d7095b61b9a07cfd2cbc9546a3c7c86R394

[fzhedu]

rest LGTM

[windtalker]

what about this case? https://github.com/pingcap/tidb/pull/23078/files#diff-dec6808c7659579faf2c9c6f9d8b7a7e0d7095b61b9a07cfd2cbc9546a3c7c86R394

This is batchCop related, MPP query will never reach there.

[LittleFall]

LGTM

[windtalker]

/run-check_dev_2

[LittleFall]

@LittleFall Why will the pr eliminate the origin error message?(or do you mean that #22459 will eliminate the origin error message). If fallback doesn't take effect, then the user receives the origin error message. If fallback takes effect, tidb server logs the original error and the user receives TiFlash server timeout warning.
Besides, when the query is allowed to fall back, maybe adding the original error in the warnings is better than adding the unified error TiFlash server timeout.

You are right, we should not cover up the original error with tiflash server timeout. This is what 23078 does, not this pr.

I want to explain if we want to change the behavior back (throw the original error, instead of tiflash server timeout) in this pr, Do you have any suggestions about the implementation?

it's not this pr's fault, record a issue: #29396

[fzhedu]

LGTM

[fzhedu]

what about this case? https://github.com/pingcap/tidb/pull/23078/files#diff-dec6808c7659579faf2c9c6f9d8b7a7e0d7095b61b9a07cfd2cbc9546a3c7c86R394

This is batchCop related, MPP query will never reach there.

remote read can not reach here?

[windtalker]

/merge

[ti-chi-bot]

This pull request has been accepted and is ready to merge.

Commit hash: af6f452

[windtalker]

/run-check_dev_2

[xuyifangreeneyes]

I want to explain if we want to change the behavior back (throw the original error, instead of tiflash server timeout) in this pr, Do you have any suggestions about the implementation?

I haven't thought of a good way to solve the problem. Maybe we should list the errors that need fallback rather than convert all the errors to ErrTiFlashServerTimeout. But the errors can be different errors (such as rpc errors and so on) and it may be hard to list them. At least showing the original error instead of ErrTiFlashServerTimeout in the warnings as #29396 suggests is an improvement.