sql/catalog/hydratedtables: add a cache for hydrated table descriptors and adopt #53323
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/hydrated-desc-cache
branch
from
0481dab to
79edd01
Compare
rohany
suggested changes
Aug 24, 2020
ajwerner
force-pushed
the
ajwerner/hydrated-desc-cache
branch
3 times, most recently
from
bc63cd6 to
f2fea7c
Compare
rohany
reviewed
Aug 24, 2020
rohany
approved these changes
Aug 24, 2020
ajwerner
force-pushed
the
ajwerner/hydrated-desc-cache
branch
from
f2fea7c to
6f2da78
Compare
|
Good new is I benchmarked this and found: Still working though the logictest changes |
|
nice, glad to see this measurable impact |
ajwerner
force-pushed
the
ajwerner/hydrated-desc-cache
branch
2 times, most recently
from
75fc91f to
cdc0963
Compare
|
Okay @rohany I did less drastic things than at one point we discussed. I think this now works PTAL. |
rohany
reviewed
Aug 26, 2020
rohany
suggested changes
Aug 27, 2020
| // This case should not meaningfully arise in practice. | ||
| td := tableDescUDT.ImmutableCopy().(*tabledesc.Immutable) | ||
| { | ||
| hydrated, canUse, err := c.GetHydratedTableDescriptor(ctx, td, resWithMut) |
There was a problem hiding this comment.
why can we use cache in this case and below? Isn't the type modified
There was a problem hiding this comment.
Can use does not mean that we definitely stored it in the cache, just that we hydrated it underneath the call.
…table Release note: None
Adds an IsHydrated method to types and then uses it to determine whether a type should be hydrated. Release note: None
This is handy when trying to determine whether we can cache a descriptor. Release justification: Part of a change to eliminate a major performance degradation for a new feature. Release note: None
ajwerner
force-pushed
the
ajwerner/hydrated-desc-cache
branch
from
cdc0963 to
d41f79f
Compare
rohany
approved these changes
Aug 27, 2020
This cache ends up being important for dealing with user-defined types. Release note: None Release justification:
Release note: None
When we hydrate a table descriptor which has been modified during the transaction we want to ensure that we propagate the information that indicates that it has been modified. Release justification: Part of a larger fix to a major performance enhancement for user-defined types, a new feature in this release. Release note: None
ajwerner
force-pushed
the
ajwerner/hydrated-desc-cache
branch
from
d41f79f to
2b6e7c9
Compare
|
TFTR! bors r=rohany |
|
Build failed (retrying...): |
|
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.
See individual commits.
Fixes #51385.
Release note: None