catalog: remove unneeded descriptor PostDeserializationChanges and clone descriptors lazily #105382
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 |
rafiss
force-pushed
the
lazy-clone-during-post-deserialization
branch
from
7a67d56 to
459d22e
Compare
rafiss
force-pushed
the
lazy-clone-during-post-deserialization
branch
2 times, most recently
from
9ec9009 to
83a7853
Compare
cockroachdb#81310 included an upgrade job that ran post deserialization changes on all descriptors, as part of the v22.2 upgrade. That, combined with the fact that we only need to support restoring descriptors from one major version ago, means that we can remove all these fixes and backport the removal to 23.1. Release note: None
cockroachdb#81310 included an upgrade job that ran post deserialization changes on all descriptors, as part of the v22.2 upgrade. That, combined with the fact that we only need to support restoring descriptors from one major version ago, means that we can remove this privilege fixing logic and backport the removal to 23.1. Release note: None
A profile showed that the most expensive part of reading a large number of descriptors is the fact that we clone each one unconditionally. However, most of the time, no changes are needed, so there's no need to clone it. Release note: None
Since we only support restoring a backup from one previous major version ago, we can remove this code. Release note: None
When running RunRestoreChanges, it is expensive to clone all the descriptors. This refactor makes it so we only clone the descriptor if it needs to be modified. Release note: None
Remove the changes that are no longer needed since all descriptors were fixed in the v22.1 upgrade. For the ones that remain, change them to lazily clone the protobuf as needed, rather than doing so unconditionally. Cloning is expensive, and shows up in profiles when reading large numbers of descriptors. Release note: None
Since cloning the descriptor is expensive, we should only clone them if a modification needs to be made. Previous commits already made the change for tables and databases, so this one covers functions, types, and schemas. Release note: None
rafiss
force-pushed
the
lazy-clone-during-post-deserialization
branch
from
83a7853 to
5d2b59c
Compare
|
closing in favor of #127028 |
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.
A profile showed that the most expensive part of reading a large number
of descriptors is the fact that we clone each one unconditionally.
See individual commits.
catalog: remove unneeded table descriptor PostDeserializationChanges
#81310 included an upgrade
job that ran post deserialization changes on all descriptors, as part of
the v22.2 upgrade. That, combined with the fact that we only need to
support restoring descriptors from one major version ago, means that we
can remove all these fixes and backport the removal to 23.1.
catprivilege: remove unneeded privilege fixing logic
tabledesc: avoid cloning proto in PostDeserialization if possible
A profile showed that the most expensive part of reading a large number
of descriptors is the fact that we clone each one unconditionally.
However, most of the time, no changes are needed, so there's no need to
clone it.
tabledesc: remove unneeded changes from RunRestoreChanges
Since we only support restoring a backup from one previous major version
ago, we can remove this code.
scpb: make MigrateDescriptorState clone descriptor lazily
When running RunRestoreChanges, it is expensive to clone all
the descriptors. This refactor makes it so we only clone the descriptor
if it needs to be modified.
dbdesc: remove unneeded PostDeserializationChanges
Remove the changes that are no longer needed since all descriptors were
fixed in the v22.1 upgrade.
For the ones that remain, change them to lazily clone the protobuf as
needed, rather than doing so unconditionally. Cloning is expensive, and
shows up in profiles when reading large numbers of descriptors.
catalog: lazily clone descriptor when setting ModificationTime
Since cloning the descriptor is expensive, we should only clone them if
a modification needs to be made.
Previous commits already made the change for tables and databases, so
this one covers functions, types, and schemas.
Epic: None
Release note: None