sql: fix lease interaction with offline tables #40996
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 |
This updates the lease management code to handle offline tables the same way dropped tables are handled. We are not supposed to be able to acquire a lease on offline tables (as well as dropped tables). Release justification: Fixes test flakes where a table descriptor lease is being taken on an offline table. Release note: None
pbardea
force-pushed
the
fix-offline-leases
branch
from
1657f0c
to
573e349
Compare
dt
reviewed
Sep 23, 2019
| @@ -284,7 +288,7 @@ func (tc *TableCollection) getTableVersion( | |||
| // Read the descriptor from the store in the face of some specific errors | |||
| // because of a known limitation of AcquireByName. See the known | |||
| // limitations of AcquireByName for details. | |||
| if err == errTableDropped || err == sqlbase.ErrDescriptorNotFound { | |||
dt
approved these changes
Sep 23, 2019
|
bors r=dt |
craig bot
pushed a commit
that referenced
this pull request
Sep 23, 2019
40996: sql: fix lease interaction with offline tables r=dt a=pbardea This updates the lease management code to handle offline tables the same way dropped tables are handled. We are not supposed to be able to acquire a lease on offline tables (as well as dropped tables). Fixes #40951 and therefore #40361. Release justification: Fixes test flakes where a table descriptor lease is being taken on an offline table. Release note: None Co-authored-by: Paul Bardea <pbardea@gmail.com>
dt
reviewed
Sep 23, 2019
| @@ -1597,7 +1597,7 @@ func TestImportIntoCSV(t *testing.T) { | |||
| <-importBodyFinished | |||
|
|
|||
| err := sqlDB.DB.QueryRowContext(ctx, `SELECT 1 FROM t`).Scan(&unused) | |||
| if !testutils.IsError(err, "table \"t\" is offline: importing") { | |||
| if !testutils.IsError(err, "relation \"t\" does not exist") { | |||
There was a problem hiding this comment.
not a blocker to a correctness fix, but I did kinda like the more descriptive user-visible error message (and we'd gotten feedback that liked the importing message from others as well).
There was a problem hiding this comment.
so the summary here is that the error we were catching was the one thrown when failing to get a lease on the table. this isn't exactly what we want - we want to see the table in the offline state and return it.
the actual PR that changed the error that we're returning here is #40285. That PR changed the logic from returning an error if the table was in a DROP of OFFLINE state to just reporting the object it was looking for as not there.
There was a problem hiding this comment.
Looking in more detail, it looks like that PR (40285) doesn't actually affect this as the behaviour described (returning no object found instead of the error) was always what was happening.
pbardea
added a commit
to pbardea/cockroach
that referenced
this pull request
Sep 23, 2019
Previously a wait loop was needed in the backup2TB roachtest because the test was reporting the table as offline when it shouldn't have seen it as OFFLINE. This was fixed by cockroachdb#40996, and therefore we should no longer need this wait loop. Release justification: Only touches tests. Release note: None
Build succeeded |
|
Thanks for tracking this down @pbardea. |
|
Did you mean to close #40361 with this PR? |
|
Yep, thanks for catching that.
…On Tue, Sep 24, 2019 at 01:12 Nathan VanBenschoten ***@***.***> wrote:
Did you mean to close #40361
<#40361> with this PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#40996?email_source=notifications&email_token=AAD337VM2LKWI7UTT5YNLI3QLGOU7A5CNFSM4IZSMWLKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7NCXWA#issuecomment-534391768>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAD337T3JGOVUOG336GMYWLQLGOU7ANCNFSM4IZSMWLA>
.
|
craig bot
pushed a commit
that referenced
this pull request
Oct 8, 2019
41010: roachtest: remove wait loop in backup2TB roachtest r=pbardea a=pbardea Previously a wait loop was needed in the backup2TB roachtest because the test was reporting the table as offline when it shouldn't have seen it as OFFLINE. This was fixed by #40996, and therefore we should no longer need this wait loop. Closes #36841. Release justification: Only touches tests. Release note: None 41250: opt: map and push down equality conditions r=rytaft a=rytaft This commit adds a new normalization rule to enable pushing variable equality conditions such as `a.x=b.x` through joins. For example, consider this query: `SELECT * FROM a, b, c WHERE a.x=b.x AND a.x=c.x` Given join ordering `(a join (b join c))`, it should be possible to infer the filter `b.x=c.x` and push it down from the top level onto the join `(b join c)`. This commit enables that mapping and pushdown to happen. In addition, this commit updates the `AssociateJoin` rule to map as many equality conditions as possible to use the output columns of the new inner-most join, allowing those conditions to be pushed onto that join. For example, consider this query: `SELECT * FROM a, b, c WHERE a.x=b.x AND b.x=c.x` If the AssociateJoin rule creates a new join ordering `(b join (a join c))`, it should be possible to map `a.x=b.x` to `a.x=c.x` and add it onto the new inner-most join `(a join c)`. This commit enables that mapping to happen. Fixes #38716 Fixes #36226 Release note (performance improvement): Improved performance for some join queries due to improved filter inference during planning. Release justification: This commit will not be merged before the release branch is cut. Co-authored-by: Paul Bardea <pbardea@gmail.com> Co-authored-by: Rebecca Taft <becca@cockroachlabs.com>
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.
This updates the lease management code to handle offline tables the
same way dropped tables are handled. We are not supposed to be able
to acquire a lease on offline tables (as well as dropped tables).
Fixes #40951 and therefore #40361.
Release justification: Fixes test flakes where a table descriptor lease
is being taken on an offline table.
Release note: None