sql/catalog/resolver: return UndefinedDatabase in more cases #71440
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 |
ajwerner
force-pushed
the
ajwerner/fix-some-error-returns-for-missing-database
branch
from
73ce1a6 to
c70ea9a
Compare
rafiss
approved these changes
Oct 12, 2021
| if found, prefix, result, err = r.LookupObject( | ||
| ctx, lookupFlags, curDb, u.Schema(), u.Object(), | ||
| ); found || err != nil || isVirtualSchema { | ||
| if !found && err == nil && prefix.Database == nil { // && isVirtualSchema |
There was a problem hiding this comment.
heh it was intentional, I'm noting that that condition is implied. I'll make that clearer.
In 21.1 and prior, we returned UndefinedDatabase (3D000) in cases where a table name was used to signify a virtual table and the database did not exist. It turns out that this behavior was important. It existed for arbitrary implementation reasons. When that code was refactored earlier in the 21.2 release, the behavior was removed. Now we restore that behavior and make the behavior more consistent. When we know that we're trying to resolve a relation in the current database and we know that the current database does not exist, we now return the UndefinedDatabase error. Release note (bug fix): In prior betas of 21.2, some error codes returned when looking for a descriptor in a non-existent database were changed from UndefinedDatabase (3D000) to UndefinedObject (42704). The behavior has been reverted and more generally name resolution when the current database is undefined will return UndefinedDatabase.
ajwerner
force-pushed
the
ajwerner/fix-some-error-returns-for-missing-database
branch
from
c70ea9a to
a7c58d6
Compare
|
@fqazi can I have a review from you, you touched this area. |
fqazi
approved these changes
Oct 14, 2021
There was a problem hiding this comment.
Reviewed 6 of 7 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @rafiss)
|
TFTR! bors r+ |
|
Build failed: |
|
bors r+ |
|
Build succeeded: |
|
Change https://review.dev.storj.io/c/storj/gateway-mt/+/6498 mentions this issue. |
storjBuildBot
pushed a commit
to storj/edge
that referenced
this pull request
Dec 10, 2021
This change partially reverts d176707 and fixes the actual underlying problem of the failing test. The reason we were failing with CRDB v21.2.x is that it now returns the 3D000 error code instead of 42P01 when the database does not exist (which happens when we run AOST queries right after migration). Previously, we only checked 42P01 before we errored or fell back to a query without AOST. The d176707-fix was incorrect because we have a fallback to a non-AOST query implemented. Reference: cockroachdb/cockroach#71440 Change-Id: Ic4b83d40579c983c8f1f5b21f3afd833d7e6057f
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.
In 21.1 and prior, we returned UndefinedDatabase (3D000) in cases where a
table name was used to signify a virtual table and the database did not
exist. It turns out that this behavior was important. It existed for
arbitrary implementation reasons. When that code was refactored
earlier in the 21.2 release, the behavior was removed. Now we restore
that behavior and make the behavior more consistent. When we know that
we're trying to resolve a relation in the current database and we know
that the current database does not exist, we now return the
UndefinedDatabase error.
Fixes #71495
Release note (bug fix): In prior betas of 21.2, some error codes returned when
looking for a descriptor in a non-existent database were changed from
UndefinedDatabase (3D000) to UndefinedObject (42704). The behavior has been
reverted and more generally name resolution when the current database is
undefined will return UndefinedDatabase.