opt: catch all pgerror.Error in optbuilder #35736
Merged
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
|
This change is |
rytaft
approved these changes
Mar 14, 2019
There was a problem hiding this comment.
Reviewed 18 of 18 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @knz)
knz
approved these changes
Mar 14, 2019
There was a problem hiding this comment.
Please edit the linters too, to match the new format. Thanks
LGTM otherwise
Reviewable status:
We now catch all `pgerror.Error`s in optbuilder, which means that we don't need to use the `buildError` wrapper with them. The wrapper still exists when external calls (e.g. for name resolution) return a generic error. The main motivation is that optbuilder calls into the factory which can panic internally. We will want to switch those panics to assertion errors as well, but they are in different packages. The existing approach would have required a shared, exported wrapper. Release note: None
RaduBerinde
force-pushed
the
optbuilder-panics-refine
branch
from
dcbf836 to
c576561
Compare
|
Heh, the linter didn't need fixing for this change (other than updating the message), but I found that the pattern had a bug and it missed a bunch of |
|
bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Mar 14, 2019
35688: sql: specify hash or merge join in EXPLAIN r=RaduBerinde a=RaduBerinde Replace `join` with `hash-join` or `merge-join` in `EXPLAIN` output. Fixes #35683. Release note (sql change): EXPLAIN tree now uses `hash-join` or `merge-join` instead of `join`. 35719: roachpb: prevent data race on Transaction r=nvanbenschoten a=nvanbenschoten Fixes #34241. This PR starts off with a series of small cleanups related to ownership of `roachpb.Transaction` objects and the use of deep and shallow clones. This makes up the first 5 commits. Next, the 6th commit removes redundant calls to `Transaction.UpdateObservedTimestamp`, reducing it down to have just a single caller that is easy to reason about. The 7th commit is what actually fixes the referenced issue. It adds in the proto clone that was missing from `BatchResponse_Header.combine` and allowing a `BatchRequest` to reference the same `Transaction` object as a `BatchResponse`. I confirmed a number of times that this stops the assertion from firing, so the commit also re-enables the skipped roachtest and removes the assertion. ~The final two commits are the two that we might consider waiting on and not including in this release, but they're also the most exciting. By making `Transaction.ObservedTimestamps` immutable (which it almost already was), we can prohibit all interior mutability of references within `Transaction`, give it value semantics, and eliminate the distinction between "shallow" and "deep" object cloning. This reduces the cost of a clone to a single straightforward allocation and makes working with the object easier to think about.~ EDIT: these two were removed from this PR. Past this point, I think the only other change we might want to make here is making a clone of `ba.Txn` in `internalClientAdapter` and then declare that the `Batch` handler goroutine has exclusive ownership over its `ba.Txn`. This would allow us to avoid a few conservative clones that would no longer be necessary, like [here](https://github.com/cockroachdb/cockroach/blob/57e825a7940495b67e0cc7213a5fabc24e12be0e/pkg/storage/store.go#L2827) and [here](https://github.com/cockroachdb/cockroach/blob/57e825a7940495b67e0cc7213a5fabc24e12be0e/pkg/storage/replica.go#L1309). I did not make this change here. 35736: opt: catch all pgerror.Error in optbuilder r=RaduBerinde a=RaduBerinde We now catch all `pgerror.Error`s in optbuilder, which means that we don't need to use the `buildError` wrapper with them. The wrapper still exists when external calls (e.g. for name resolution) return a generic error. The main motivation is that optbuilder calls into the factory which can panic internally. We will want to switch those panics to assertion errors as well, but they are in different packages. The existing approach would have required a shared, exported wrapper. Release note: None Co-authored-by: Radu Berinde <[email protected]> Co-authored-by: Nathan VanBenschoten <[email protected]>
Build succeeded |
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.
We now catch all
pgerror.Errors in optbuilder, which means that wedon't need to use the
buildErrorwrapper with them. The wrapperstill exists when external calls (e.g. for name resolution) return a
generic error.
The main motivation is that optbuilder calls into the factory which
can panic internally. We will want to switch those panics to assertion
errors as well, but they are in different packages. The existing
approach would have required a shared, exported wrapper.
Release note: None