From bba05aabcbba78dbec0a309f0b96440cb8b71d26 Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Mon, 25 Oct 2021 12:31:43 -0400 Subject: [PATCH] sql/catalog/descs: optimize access to the system db MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We tend to access the system database a lot and we never cache it in the lease manager. Before this patch, we'd always copy and re-allocate a pair of them. There's no need to do that. ``` name old time/op new time/op delta FlowSetup/vectorize=true/distribute=true-16 141µs ± 3% 132µs ± 4% -6.35% (p=0.000 n=19+18) FlowSetup/vectorize=true/distribute=false-16 138µs ± 4% 129µs ± 3% -6.80% (p=0.000 n=19+18) FlowSetup/vectorize=false/distribute=true-16 134µs ± 2% 124µs ± 4% -7.55% (p=0.000 n=20+17) FlowSetup/vectorize=false/distribute=false-16 129µs ± 3% 120µs ± 3% -6.98% (p=0.000 n=20+18) name old alloc/op new alloc/op delta FlowSetup/vectorize=true/distribute=true-16 38.1kB ± 2% 36.8kB ± 3% -3.53% (p=0.000 n=18+19) FlowSetup/vectorize=true/distribute=false-16 36.2kB ± 0% 34.8kB ± 0% -3.93% (p=0.000 n=17+17) FlowSetup/vectorize=false/distribute=true-16 42.6kB ± 0% 41.2kB ± 0% -3.30% (p=0.000 n=18+16) FlowSetup/vectorize=false/distribute=false-16 41.0kB ± 0% 39.6kB ± 0% -3.44% (p=0.000 n=16+18) name old allocs/op new allocs/op delta FlowSetup/vectorize=true/distribute=true-16 368 ± 0% 345 ± 0% -6.25% (p=0.000 n=16+16) FlowSetup/vectorize=true/distribute=false-16 354 ± 0% 331 ± 0% -6.50% (p=0.000 n=18+18) FlowSetup/vectorize=false/distribute=true-16 337 ± 0% 315 ± 1% -6.69% (p=0.000 n=19+19) FlowSetup/vectorize=false/distribute=false-16 325 ± 0% 302 ± 0% -7.08% (p=0.000 n=17+18) ``` Release note: None --- .../catalog/descs/uncommitted_descriptors.go | 43 +++++++++++++++++-- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/pkg/sql/catalog/descs/uncommitted_descriptors.go b/pkg/sql/catalog/descs/uncommitted_descriptors.go index f3712dbe5c97..89d9317a414b 100644 --- a/pkg/sql/catalog/descs/uncommitted_descriptors.go +++ b/pkg/sql/catalog/descs/uncommitted_descriptors.go @@ -12,9 +12,12 @@ package descs import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/lease" "github.com/cockroachdb/cockroach/pkg/sql/catalog/nstree" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema" "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" "github.com/cockroachdb/cockroach/pkg/util/iterutil" "github.com/cockroachdb/errors" @@ -23,8 +26,14 @@ import ( // uncommittedDescriptor is a descriptor that has been modified in the current // transaction. type uncommittedDescriptor struct { - mutable catalog.MutableDescriptor immutable catalog.Descriptor + + // mutable generally holds the descriptor as it was read from the database. + // In the rare case that this struct corresponds to a singleton which is + // added to optimize for special cases of system descriptors. + // It should be accessed through getMutable() which will construct a new + // value in cases where it is nil. + mutable catalog.MutableDescriptor } // GetName implements the catalog.NameEntry interface. @@ -47,6 +56,16 @@ func (u uncommittedDescriptor) GetID() descpb.ID { return u.immutable.GetID() } +// getMutable is how the mutable descriptor should be accessed. It constructs +// a new descriptor in the case that this descriptor is a cached, in-memory +// singleton for a system descriptor. +func (u *uncommittedDescriptor) getMutable() catalog.MutableDescriptor { + if u.mutable != nil { + return u.mutable + } + return catalogkv.NewBuilder(u.immutable.DescriptorProto()).BuildExistingMutable() +} + var _ catalog.NameEntry = (*uncommittedDescriptor)(nil) // uncommittedDescriptors is the data structure holding all @@ -113,6 +132,7 @@ func (ud *uncommittedDescriptors) add(mut catalog.MutableDescriptor) (catalog.De // checkOut checks out an uncommitted mutable descriptor for use in the // transaction. This descriptor should later be checked in again. func (ud *uncommittedDescriptors) checkOut(id descpb.ID) (catalog.MutableDescriptor, error) { + ud.maybeInitialize() entry := ud.descs.GetByID(id) if entry == nil { return nil, errors.NewAssertionErrorWithWrappedErrf( @@ -122,7 +142,7 @@ func (ud *uncommittedDescriptors) checkOut(id descpb.ID) (catalog.MutableDescrip } u := entry.(*uncommittedDescriptor) - return u.mutable, nil + return u.getMutable(), nil } // checkIn checks in an uncommitted mutable descriptor that was previously @@ -170,6 +190,7 @@ func maybeRefreshCachedFieldsOnTypeDescriptor( // getByID looks up an uncommitted descriptor by ID. func (ud *uncommittedDescriptors) getByID(id descpb.ID) catalog.Descriptor { + ud.maybeInitialize() entry := ud.descs.GetByID(id) if entry == nil { return nil @@ -187,6 +208,7 @@ func (ud *uncommittedDescriptors) getByID(id descpb.ID) catalog.Descriptor { func (ud *uncommittedDescriptors) getByName( dbID descpb.ID, schemaID descpb.ID, name string, ) (hasKnownRename bool, desc catalog.Descriptor) { + ud.maybeInitialize() // Walk latest to earliest so that a DROP followed by a CREATE with the same // name will result in the CREATE being seen. if got := ud.descs.GetByName(dbID, schemaID, name); got != nil { @@ -202,9 +224,10 @@ func (ud *uncommittedDescriptors) getByName( func (ud *uncommittedDescriptors) iterateNewVersionByID( fn func(entry catalog.NameEntry, originalVersion lease.IDVersion) error, ) error { + ud.maybeInitialize() return ud.descs.IterateByID(func(entry catalog.NameEntry) error { mut := entry.(*uncommittedDescriptor).mutable - if mut.IsNew() || !mut.IsUncommittedVersion() { + if mut == nil || mut.IsNew() || !mut.IsUncommittedVersion() { return nil } return fn(entry, lease.NewIDVersionPrev(mut.OriginalName(), mut.OriginalID(), mut.OriginalVersion())) @@ -214,6 +237,7 @@ func (ud *uncommittedDescriptors) iterateNewVersionByID( func (ud *uncommittedDescriptors) iterateImmutableByID( fn func(imm catalog.Descriptor) error, ) error { + ud.maybeInitialize() return ud.descs.IterateByID(func(entry catalog.NameEntry) error { return fn(entry.(*uncommittedDescriptor).immutable) }) @@ -262,3 +286,16 @@ func (ud *uncommittedDescriptors) hasUncommittedTypes() (has bool) { }) return has } + +var systemUncommittedDatabase = &uncommittedDescriptor{ + immutable: dbdesc.NewBuilder(systemschema.SystemDB.DatabaseDesc()). + BuildImmutableDatabase(), + // Note that the mutable field is left as nil. We'll generate a new + // value lazily when this is needed, which ought to be exceedingly rare. +} + +func (ud *uncommittedDescriptors) maybeInitialize() { + if ud.descs.Len() == 0 { + ud.descs.Upsert(systemUncommittedDatabase) + } +}