release-19.1: pgerror: avoid wrapping special roachpb errors #35939
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Prior to this patch, the following scenario was possible:
1. some SQL function A() calls client.Txn() to call B() on its behalf
with a retry loop.
2. client.Txn() calls B()
3. B() runs KV operation, encounters some *roachpb.RetryError
4. B() calls `pgerror.Wrap()` on that retry error, turns it into pg
error with code 40001
5. B() returns into client.Txn()
6. client.Txn() tests the type of the error expecting to find a
roachpb error for retries. Because it's a pgerror now, the test
fails and the retry error is not handled automatically. It flows
out.
7. A() receives a retry error when it was not expecting any.
The error would incur spurious DDL failures, with the following
example:
```
--- FAIL: TestLogic/fakedist-opt/computed (2.66s)
logic.go:2346:
testdata/logic_test/computed:687:
expected:
division by zero
got:
pq: column "d" being dropped, try again later
```
(This particular test would fail after just a few attempts during a
stress run.)
This patch fixes this scenario by ensuring that retry and ambiguous
result errors flow through `pgerror.Wrapf` without being flattened
into a `pgerror.Error`, and instead remain wrapped as the original
cause via `errors.Wrapf`.
The patch is verified to work by stress testing the logic tests and
verifying that the various error symptoms do not occur any more.
Additionally, two missing calls to `errors.Cause()` were added to
ensure any enclosed roachpb error is visible to a type check.
Release note: None
|
This change is |
|
LGTM but wait until the other one has merged please.
…On Tue, Mar 19, 2019 at 2:55 PM kena ***@***.***> wrote:
@knz <https://github.com/knz> requested your review on: #35939
<#35939> release-19.1:
pgerror: avoid wrapping special roachpb errors.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#35939 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AE135GwH3yDtLjZjxw3aNwqDjZDWdOmMks5vYOxXgaJpZM4b8MX6>
.
|
This was referenced Mar 19, 2019
Works for me. I assume you also want a green master build? |
|
No, just for it to have merged to master (there have been cases where we
merged the backport before the original, and then some change had to be
made to the original, etc...)
…On Tue, Mar 19, 2019 at 3:17 PM kena ***@***.***> wrote:
LGTM but wait until the other one has merged please.
Works for me. I assume you also want a green master build?
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
<#35939 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AE135M-l4tzJwoTQIWB0ibd8BsmG8gxSks5vYPF1gaJpZM4b8MX6>
.
|
Oh I see. Well it's merged now. |
|
... and we're getting another totally unrelated failure here. I'm going to check whether my patch is responsible for it. |
This patch ensures that the full error message (pre-unwrap) is included for errors that flow out of distsql process towards the gateway. This is important because some other internal mechanisms inside CockroachDB are expecting to see the prefixes introduced by errors.Wrap to decide other things (a bad idea, but not fixable in the short term), for example see issue cockroachdb#35942. Release note: None
|
LGTM |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport 1/1 commits from #35937.
Backport 1/1 commits from #35944.
/cc @cockroachdb/release
Fixes #35883.
Fixes #35943