Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sql: let AS OF SYSTEM TIME requests use table descriptor cache #31716

Merged
merged 1 commit into from
Oct 23, 2018
Merged

sql: let AS OF SYSTEM TIME requests use table descriptor cache #31716

merged 1 commit into from
Oct 23, 2018

Conversation

vivekmenezes
Copy link
Contributor

This change also fixes a problem where TRUNCATE was not
setting the ModificationTime on the descriptor.

This change also fixes a bug where we were not using
Timestamp.Prev() to compute the previous timestamp
when reading the previous version of a descriptor.

This change disables the use of the descriptor cache
when AS OF SYSTEM TIME is for a timestamp before
a DROP or a TRUNCATE on a table because of a bug
in AcquireByName().

fixes #30967

Release note (sql change): Speedup AS OF SYSTEM TIME requests by
letting them use the table descriptor cache.

@vivekmenezes vivekmenezes requested review from dt and a team October 23, 2018 03:32
@vivekmenezes
Copy link
Contributor Author

FYI @nvanbenschoten

@dt dt requested a review from maddyblue October 23, 2018 12:29
nvanbenschoten
nvanbenschoten approved these changes Oct 23, 2018
View reviewed changes
Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting this out so fast! I'm a little concerned about the general lack of testing around here, but perhaps I'm just missing it.

@vivekmenezes
Copy link
Contributor Author

This code is tested through TestAsOfTime and TestTxnCanStillResolveOldName, so the approach is correct. The only problem with this change is that it's not testing that the cache is indeed being used. Let me add a test just for that.

@vivekmenezes
Copy link
Contributor Author

Added TestAsOfSystemTimeUsingCache

@vivekmenezes
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Oct 23, 2018

Build failed

@vivekmenezes
sql: let AS OF SYSTEM TIME requests use table descriptor cache
b251be7 
This change also fixes a problem where TRUNCATE was not
setting the ModificationTime on the descriptor.

This change also fixes a bug where we were not using
Timestamp.Prev() to compute the previous timestamp
when reading the previous version of a descriptor.

This change disables the use of the descriptor cache
when AS OF SYSTEM TIME is for a timestamp before
a DROP or a TRUNCATE on a table because of a bug
in AcquireByName().

fixes cockroachdb#30967

Release note (sql change): Speedup AS OF SYSTEM TIME requests by
letting them use the table descriptor cache.
@vivekmenezes
Copy link
Contributor Author

bors r+

craig bot pushed a commit that referenced this pull request Oct 23, 2018
@spencerkimball @vivekmenezes
Merge #31644 #31716
43ef817 
31644: storage: remove excessive new range lease logging r=spencerkimball a=spencerkimball

Release note: None

31716: sql: let AS OF SYSTEM TIME requests use table descriptor cache r=vivekmenezes a=vivekmenezes

This change also fixes a problem where TRUNCATE was not
setting the ModificationTime on the descriptor.

This change also fixes a bug where we were not using
Timestamp.Prev() to compute the previous timestamp
when reading the previous version of a descriptor.

This change disables the use of the descriptor cache
when AS OF SYSTEM TIME is for a timestamp before
a DROP or a TRUNCATE on a table because of a bug
in AcquireByName().

fixes #30967

Release note (sql change): Speedup AS OF SYSTEM TIME requests by
letting them use the table descriptor cache.

Co-authored-by: Spencer Kimball <spencer.kimball@gmail.com>
Co-authored-by: Vivek Menezes <vivek@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Oct 23, 2018

Build succeeded

@craig craig bot merged commit b251be7 into cockroachdb:master Oct 23, 2018
@vivekmenezes vivekmenezes mentioned this pull request Oct 23, 2018
Merged
vivekmenezes added a commit to vivekmenezes/cockroach that referenced this pull request Oct 24, 2018
@vivekmenezes
sql: cleanup AS OF SYSTEM TIME query descriptor cache lookup
c203240 
This change removes the fixedTimestamp bit introduced in cockroachdb#31716

It also explains problem cockroachdb#18354 with LeaseManager.Acquire() through
a comment and implements a work around bypassing the cache and
reading the descriptor directly from the store.

It also fixes a problem with UncachedPhysicalAccessor.GetObjectDesc
tested through TestTxnCanStillResolveOldName.

related to cockroachdb#30967

fixes cockroachdb#18354
fixes cockroachdb#19925

Release note: None
@vivekmenezes vivekmenezes mentioned this pull request Oct 24, 2018
Merged
@vivekmenezes vivekmenezes deleted the asof branch October 25, 2018 17:28
vivekmenezes added a commit to vivekmenezes/cockroach that referenced this pull request Oct 30, 2018
@vivekmenezes
sql: cleanup AS OF SYSTEM TIME query descriptor cache lookup
4d9ebc2 
This change removes the fixedTimestamp bit introduced in cockroachdb#31716

It also explains problem cockroachdb#18354 with LeaseManager.Acquire() through
a comment and implements a work around bypassing the cache and
reading the descriptor directly from the store.

It also fixes a problem with UncachedPhysicalAccessor.GetObjectDesc
tested through TestTxnCanStillResolveOldName.

related to cockroachdb#30967

fixes cockroachdb#18354
fixes cockroachdb#19925

Release note: None
vivekmenezes added a commit to vivekmenezes/cockroach that referenced this pull request Nov 1, 2018
@vivekmenezes
sql: cleanup AS OF SYSTEM TIME query descriptor cache lookup
7c9b907 
This change removes the fixedTimestamp bit introduced in cockroachdb#31716

It also explains problem cockroachdb#18354 with LeaseManager.Acquire() through
a comment and implements a work around bypassing the cache and
reading the descriptor directly from the store.

It also fixes a problem with UncachedPhysicalAccessor.GetObjectDesc
tested through TestTxnCanStillResolveOldName.

related to cockroachdb#30967

fixes cockroachdb#18354
fixes cockroachdb#19925

Release note: None
craig bot pushed a commit that referenced this pull request Nov 1, 2018
@vivekmenezes
Merge #31776
b05b2f8 
31776: sql: cleanup AS OF SYSTEM TIME query descriptor cache lookup r=vivekmenezes a=vivekmenezes

This change removes the fixedTimestamp bit introduced in #31716

It also explains problem #18354 with LeaseManager.Acquire() through
a comment and implements a work around bypassing the cache and
reading the descriptor directly from the store.

It also fixes a problem with UncachedPhysicalAccessor.GetObjectDesc
tested through TestTxnCanStillResolveOldName.

related to #30967

fixes #18354
fixes #19925

Release note: None

Co-authored-by: Vivek Menezes <vivek@cockroachlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dt dt dt approved these changes

@nvanbenschoten nvanbenschoten nvanbenschoten approved these changes

@maddyblue maddyblue Awaiting requested review from maddyblue

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sql: use descriptor cache for AS OF SYSTEM TIME
3 participants
@vivekmenezes @dt @nvanbenschoten