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.

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

catalog/descs: use btrees to store leased descriptors #51997

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thoszhang
Copy link
Contributor

This commit replaces the internal representation of leasedDescriptors
in the descriptor collection with a pair of btrees, to support faster
resolution of descriptors by name and ID.

The first commit is #51996.

Release note: None

This commit introduces a type `leasedDescriptors` to wrap the slice of
leased descriptors, with methods for accessing descriptors and adding
and removing them.

Release note: None
This commit replaces the internal representation of `leasedDescriptors`
in the descriptor collection with a pair of btrees, to support faster
resolution of descriptors by name and ID.

Release note: None
@cockroach-teamcity
Copy link
Member

This change is Reviewable

toRelease = append(toRelease, i.(descriptorWithID).Descriptor)
return true
})
ld.byName.Clear(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] add a comment for these true arguments

@@ -58,6 +181,10 @@ func MakeCollection(
dbCacheSubscriber DatabaseCacheSubscriber,
) Collection {
return Collection{
leasedDescriptors: leasedDescriptors{
byName: btree.New(2),
Copy link
Contributor

Choose a reason for hiding this comment

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

How did you pick this degree (not that I have a suggestion for another value)

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @lucy-zhang)


pkg/sql/catalog/descs/collection.go, line 52 at r2 (raw file):

	immutable catalog.Descriptor
}

This ends up being pretty crufty, huh.


pkg/sql/catalog/descs/collection.go, line 130 at r2 (raw file):

func (ld *leasedDescriptors) releaseAll() (toRelease []catalog.Descriptor) {
	defer ld.assertConsistency()
	ld.byID.Ascend(func(i btree.Item) bool {

total nit: preallocate toRelease


pkg/sql/catalog/descs/collection.go, line 139 at r2 (raw file):

}

func (ld *leasedDescriptors) release(ids []sqlbase.ID) (toRelease []catalog.Descriptor) {

consider changing this API now to just take one at a time as there isn't really an efficiency win in terms of algorithmic performance given the sorting and this will allow you to avoid allocating the ids slice and the cruft that went with it.

@tbg tbg added the X-noremind Bots won't notify about PRs with X-noremind label May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
X-noremind Bots won't notify about PRs with X-noremind
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants