From d58ec7ad9f2f465557ec5e152483d986adc8e9f6 Mon Sep 17 00:00:00 2001 From: Ricky Stewart Date: Thu, 11 Nov 2021 15:48:24 -0600 Subject: [PATCH 1/5] bazel: properly generate `.eg.go` code in `pkg/sql/colconv` via bazel Release note: None --- pkg/sql/colconv/BUILD.bazel | 45 ++++++++++++++++------------ pkg/sql/colconv/datum_to_vec.eg.go | 7 +++++ pkg/sql/colconv/datum_to_vec_tmpl.go | 9 ++++++ pkg/sql/colconv/vec_to_datum.eg.go | 10 +++++++ pkg/sql/colconv/vec_to_datum_tmpl.go | 15 ++++++++++ 5 files changed, 67 insertions(+), 19 deletions(-) diff --git a/pkg/sql/colconv/BUILD.bazel b/pkg/sql/colconv/BUILD.bazel index 7c0b7af019d3..85aa495b62cf 100644 --- a/pkg/sql/colconv/BUILD.bazel +++ b/pkg/sql/colconv/BUILD.bazel @@ -1,36 +1,28 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") +load("//pkg/sql/colexecop:EXECGEN.bzl", "eg_go_filegroup", "gen_eg_go_rules") -# TODO(irfansharif): The dependency tree for *.eg.go needs -# sorting out. It depends on execgen+templates from elsewhere. Look towards -# colexec for how this should be done. For now we just lazily depend on the -# already generated+checked in file. -# -# keep go_library( name = "colconv", srcs = [ - "batch.go", # keep - "datum_to_vec.eg.go", - "vec_to_datum.eg.go", + "batch.go", + ":gen-exec", # keep ], importpath = "github.com/cockroachdb/cockroach/pkg/sql/colconv", visibility = ["//visibility:public"], - # Pin dependencies used by auto-generated code. deps = [ "//pkg/col/coldata", - "//pkg/col/coldataext", - "//pkg/col/typeconv", - "//pkg/sql/colexecerror", + "//pkg/col/typeconv", # keep + "//pkg/sql/colexecerror", # keep "//pkg/sql/execinfra", # keep - "//pkg/sql/rowenc", - "//pkg/sql/sem/tree", - "//pkg/sql/types", + "//pkg/sql/rowenc", # keep + "//pkg/sql/sem/tree", # keep + "//pkg/sql/types", # keep "//pkg/util/encoding", # keep "//pkg/util/json", # keep - "//pkg/util/timeutil/pgdate", - "//pkg/util/uuid", + "//pkg/util/timeutil/pgdate", # keep + "//pkg/util/uuid", # keep "@com_github_cockroachdb_errors//:errors", # keep - "@com_github_lib_pq//oid", + "@com_github_lib_pq//oid", # keep ], ) @@ -45,3 +37,18 @@ go_test( "@com_github_stretchr_testify//require", ], ) + +# Map between target name and relevant template. +targets = [ + ("datum_to_vec.eg.go", "datum_to_vec_tmpl.go"), + ("vec_to_datum.eg.go", "vec_to_datum_tmpl.go"), +] + +# Define a file group for all the .eg.go targets. +eg_go_filegroup( + name = "gen-exec", + targets = targets, +) + +# Define gen rules for individual eg.go files. +gen_eg_go_rules(targets) diff --git a/pkg/sql/colconv/datum_to_vec.eg.go b/pkg/sql/colconv/datum_to_vec.eg.go index 2352f76cdfea..0df6701f3913 100644 --- a/pkg/sql/colconv/datum_to_vec.eg.go +++ b/pkg/sql/colconv/datum_to_vec.eg.go @@ -18,6 +18,13 @@ import ( "github.com/cockroachdb/errors" ) +// Workaround for bazel auto-generated code. goimports does not automatically +// pick up the right packages when run within the bazel sandbox. +var ( + _ encoding.Direction + _ = typeconv.DatumVecCanonicalTypeFamily +) + // GetDatumToPhysicalFn returns a function for converting a datum of the given // ColumnType to the corresponding Go type. Note that the signature of the // return function doesn't contain an error since we assume that the conversion diff --git a/pkg/sql/colconv/datum_to_vec_tmpl.go b/pkg/sql/colconv/datum_to_vec_tmpl.go index 7702338f2a25..639a7fbf4e85 100644 --- a/pkg/sql/colconv/datum_to_vec_tmpl.go +++ b/pkg/sql/colconv/datum_to_vec_tmpl.go @@ -22,12 +22,21 @@ package colconv import ( + "github.com/cockroachdb/cockroach/pkg/col/typeconv" "github.com/cockroachdb/cockroach/pkg/sql/colexecerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" + "github.com/cockroachdb/cockroach/pkg/util/encoding" "github.com/cockroachdb/errors" ) +// Workaround for bazel auto-generated code. goimports does not automatically +// pick up the right packages when run within the bazel sandbox. +var ( + _ encoding.Direction + _ = typeconv.DatumVecCanonicalTypeFamily +) + // GetDatumToPhysicalFn returns a function for converting a datum of the given // ColumnType to the corresponding Go type. Note that the signature of the // return function doesn't contain an error since we assume that the conversion diff --git a/pkg/sql/colconv/vec_to_datum.eg.go b/pkg/sql/colconv/vec_to_datum.eg.go index 3778faf68193..c791b31702ee 100644 --- a/pkg/sql/colconv/vec_to_datum.eg.go +++ b/pkg/sql/colconv/vec_to_datum.eg.go @@ -26,6 +26,16 @@ import ( "github.com/lib/pq/oid" ) +// Workaround for bazel auto-generated code. goimports does not automatically +// pick up the right packages when run within the bazel sandbox. +var ( + _ colexecerror.StorageError + _ json.JSON + _ pgdate.Date + _ = typeconv.DatumVecCanonicalTypeFamily + _ uuid.UUID +) + // VecToDatumConverter is a helper struct that converts vectors from batches to // their datum representations. // TODO(yuzefovich): the result of converting the vectors to datums is usually diff --git a/pkg/sql/colconv/vec_to_datum_tmpl.go b/pkg/sql/colconv/vec_to_datum_tmpl.go index 38199b8fe6f6..039e8c1f0d9c 100644 --- a/pkg/sql/colconv/vec_to_datum_tmpl.go +++ b/pkg/sql/colconv/vec_to_datum_tmpl.go @@ -25,13 +25,28 @@ import ( "sync" "github.com/cockroachdb/cockroach/pkg/col/coldata" + "github.com/cockroachdb/cockroach/pkg/col/typeconv" + "github.com/cockroachdb/cockroach/pkg/sql/colexecerror" "github.com/cockroachdb/cockroach/pkg/sql/execinfra" "github.com/cockroachdb/cockroach/pkg/sql/rowenc" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" + "github.com/cockroachdb/cockroach/pkg/util/json" + "github.com/cockroachdb/cockroach/pkg/util/timeutil/pgdate" + "github.com/cockroachdb/cockroach/pkg/util/uuid" "github.com/lib/pq/oid" ) +// Workaround for bazel auto-generated code. goimports does not automatically +// pick up the right packages when run within the bazel sandbox. +var ( + _ colexecerror.StorageError + _ json.JSON + _ pgdate.Date + _ = typeconv.DatumVecCanonicalTypeFamily + _ uuid.UUID +) + // VecToDatumConverter is a helper struct that converts vectors from batches to // their datum representations. // TODO(yuzefovich): the result of converting the vectors to datums is usually From 340346dc24740414dad6065671ccd6661913b83c Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Fri, 12 Nov 2021 17:03:05 -0500 Subject: [PATCH 2/5] sql/catalog/lease: permit gaps in descriptor history In #71239, we added a new mechanism to look up historical descriptors. I erroneously informed @jameswsj10 that we would never have gaps in the descriptor history, and, thus, when looking up historical descriptors, we could always use the earliest descriptor's modification time as the bounds for the relevant query. This turns out to not be true. Consider the case where version 3 is a historical version and then version 4 pops up and gets leased. Version 3 will get removed if it is not referenced. In the meantime, version 3 existed when we went to go find version 2. At that point, we'll inject version 2 and have version 4 leased. We need to make sure we can handle the case where we need to go fetch version 3. In the meantime, this change also removes some logic added to support the eventual resurrection of #59606 whereby we'll use the export request to fetch descriptor history to power historical queries even in the face of descriptors having been deleted. Fixes #72706. Release note: None --- pkg/sql/catalog/lease/lease.go | 62 ++++++++++++++++--- pkg/sql/catalog/lease/lease_internal_test.go | 65 +++++++++++++++++--- 2 files changed, 107 insertions(+), 20 deletions(-) diff --git a/pkg/sql/catalog/lease/lease.go b/pkg/sql/catalog/lease/lease.go index 31134e1c45d4..aed32ed1d436 100644 --- a/pkg/sql/catalog/lease/lease.go +++ b/pkg/sql/catalog/lease/lease.go @@ -14,6 +14,7 @@ package lease import ( "context" "fmt" + "sort" "sync" "sync/atomic" "time" @@ -207,15 +208,23 @@ func getDescriptorsFromStoreForInterval( lowerBound, upperBound hlc.Timestamp, ) ([]historicalDescriptor, error) { // Ensure lower bound is not an empty timestamp (now). - if lowerBound.Logical == 0 && lowerBound.WallTime == 0 { - return nil, errors.New("Lower bound for export request cannot be 0") + if lowerBound.IsEmpty() { + return nil, errors.AssertionFailedf( + "getDescriptorsFromStoreForInterval: lower bound cannot be empty") + } + // TODO(ajwerner): We'll want to lift this limitation in order to allow this + // function to find descriptors which could not be found by leasing. This + // will also require some careful managing of expiration timestamps for the + // final descriptor. + if upperBound.IsEmpty() { + return nil, errors.AssertionFailedf( + "getDescriptorsFromStoreForInterval: upper bound cannot be empty") } // Create an export request (1 kv call) for all descriptors for given // descriptor ID written during the interval [timestamp, endTimestamp). - batchRequestHeader := roachpb.Header{} - if upperBound.WallTime != 0 { - batchRequestHeader = roachpb.Header{Timestamp: upperBound.Prev()} + batchRequestHeader := roachpb.Header{ + Timestamp: upperBound.Prev(), } descriptorKey := catalogkeys.MakeDescMetadataKey(codec, id) requestHeader := roachpb.RequestHeader{ @@ -308,17 +317,50 @@ func getDescriptorsFromStoreForInterval( func (m *Manager) readOlderVersionForTimestamp( ctx context.Context, id descpb.ID, timestamp hlc.Timestamp, ) ([]historicalDescriptor, error) { - // Retrieve the endTimestamp for our query, which will be the modification - // time of the first descriptor in the manager's active set. + // Retrieve the endTimestamp for our query, which will be the first + // modification timestamp above our query timestamp. t := m.findDescriptorState(id, false /*create*/) - endTimestamp := func() hlc.Timestamp { + // A missing descriptor state indicates that this descriptor has been + // purged in the meantime. We should go back around in the acquisition + // loop to make the appropriate error appear. + if t == nil { + return nil, nil + } + endTimestamp, done := func() (hlc.Timestamp, bool) { t.mu.Lock() defer t.mu.Unlock() + + // If there are no descriptors, then we won't have a valid end timestamp. if len(t.mu.active.data) == 0 { - return hlc.Timestamp{} + return hlc.Timestamp{}, true } - return t.mu.active.data[0].GetModificationTime() + // We permit gaps in historical versions. We want to find the timestamp + // that represents the start of the validity interval for the known version + // which immediately follows the timestamps we're searching for. + i := sort.Search(len(t.mu.active.data), func(i int) bool { + return timestamp.Less(t.mu.active.data[i].GetModificationTime()) + }) + + // If the timestamp we're searching for is somehow after the last descriptor + // we have in play, then either we have the right descriptor, or some other + // shenanigans where we've evicted the descriptor has occurred. + // + // TODO(ajwerner): When we come to modify this code to allow us to find + // historical descriptors which have been dropped, we'll need to rework + // this case and support providing no upperBound to + // getDescriptorFromStoreForInterval. + if i == len(t.mu.active.data) || + // If we found a descriptor that isn't the first descriptor, go and check + // whether the descriptor for which we're searching actually exists. This + // will deal with cases where a concurrent fetch filled it in for us. + i > 0 && timestamp.Less(t.mu.active.data[i-1].getExpiration()) { + return hlc.Timestamp{}, true + } + return t.mu.active.data[i].GetModificationTime(), false }() + if done { + return nil, nil + } // Retrieve descriptors in range [timestamp, endTimestamp) in decreasing // modification time order. diff --git a/pkg/sql/catalog/lease/lease_internal_test.go b/pkg/sql/catalog/lease/lease_internal_test.go index 036d74c2c3ab..3bb90a3a4adb 100644 --- a/pkg/sql/catalog/lease/lease_internal_test.go +++ b/pkg/sql/catalog/lease/lease_internal_test.go @@ -1082,11 +1082,12 @@ func TestReadOlderVersionForTimestamp(t *testing.T) { tdb.QueryRow(t, "SELECT id FROM system.namespace WHERE name = 'foo'").Scan(&tableID) manager := s.LeaseManager().(*Manager) - const N = 5 - descs := make([]catalog.Descriptor, N+1) + const numHistoricalVersions = 5 + const maxVersion = numHistoricalVersions + 1 + descs := make([]catalog.Descriptor, maxVersion) - // Create N versions of table descriptor - for i := 0; i < N; i++ { + // Create numHistoricalVersions versions of table descriptor + for i := 0; i < numHistoricalVersions; i++ { _, err := manager.Publish(ctx, tableID, func(desc catalog.MutableDescriptor) error { descs[i] = desc.ImmutableCopy() return nil @@ -1096,7 +1097,7 @@ func TestReadOlderVersionForTimestamp(t *testing.T) { { last, err := manager.Acquire(ctx, s.Clock().Now(), tableID) require.NoError(t, err) - descs[N] = last.Underlying() + descs[numHistoricalVersions] = last.Underlying() last.Release(ctx) } @@ -1126,7 +1127,11 @@ func TestReadOlderVersionForTimestamp(t *testing.T) { Descriptor: versionDesc(v), } addedDescVState.mu.Lock() - addedDescVState.mu.expiration = hlc.MaxTimestamp + if v < maxVersion { + addedDescVState.mu.expiration = versionTS(v + 1) + } else { + addedDescVState.mu.expiration = hlc.MaxTimestamp + } addedDescVState.mu.Unlock() descStates[tableID].mu.active.insert(addedDescVState) } @@ -1136,30 +1141,46 @@ func TestReadOlderVersionForTimestamp(t *testing.T) { // expected data. // [v1 ---)[v2 --)[v3 ---)[v4 ----)[v5 -----)[v6 ------) for _, tc := range []testCase{ + + // The following cases represent having no existing descriptor state, or + // as importantly, having some descriptor state but searching for a + // timestamp after the known state. The code, as stands, assumes that + // when this happens, we'll rely on an existing lease to provide the end + // timestamp, and when no such lease exists, we'll go get one. If the + // attempt to get a lease fails, then we'll propagate that error up. This + // fact is the source of the known limitation described on + // Acquire and AcquireByName. { before: []version{}, ts: versionTS(1), tsStr: "ts1", - expected: []version{1, 2, 3, 4, 5, 6}, + expected: []version{}, }, { before: []version{}, ts: versionTS(4), tsStr: "ts4", - expected: []version{4, 5, 6}, + expected: []version{}, }, { before: []version{}, ts: versionTS(6), tsStr: "ts6", - expected: []version{6}, + expected: []version{}, + }, + { + before: []version{1, 2, 3}, + ts: versionTS(4).Next(), + tsStr: "ts4.Next", + expected: []version{}, }, { before: []version{}, ts: versionTS(6).Prev(), tsStr: "ts6.Prev", - expected: []version{5, 6}, + expected: []version{}, }, + { before: []version{6}, ts: versionTS(4).Prev(), @@ -1184,6 +1205,30 @@ func TestReadOlderVersionForTimestamp(t *testing.T) { tsStr: "ts4", expected: []version{}, }, + { + before: []version{1, 4, 5, 6}, + ts: versionTS(4).Prev(), + tsStr: "ts4.Prev", + expected: []version{3}, + }, + { + before: []version{1, 4, 5, 6}, + ts: versionTS(3).Prev(), + tsStr: "ts3.Prev", + expected: []version{2, 3}, + }, + { + before: []version{1, 4, 5, 6}, + ts: versionTS(2), + tsStr: "ts2", + expected: []version{2, 3}, + }, + { + before: []version{1, 4, 5, 6}, + ts: versionTS(2).Prev(), + tsStr: "ts2.Prev", + expected: []version{}, + }, } { t.Run(fmt.Sprintf("%v@%v->%v", tc.before, tc.tsStr, tc.expected), func(t *testing.T) { // Reset the descriptor state to before versions. From 0faba2d4fc8be13b6291e6d7e574033ba2d106e2 Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Mon, 15 Nov 2021 00:21:06 -0500 Subject: [PATCH 3/5] sql/catalog/descs: fix most of perf regression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ``` name old ops/s new ops/s delta KV95-throughput 88.6k ± 0% 94.8k ± 1% +7.00% (p=0.008 n=5+5) name old ms/s new ms/s delta KV95-P50 1.60 ± 0% 1.40 ± 0% -12.50% (p=0.008 n=5+5) KV95-Avg 0.60 ± 0% 0.50 ± 0% -16.67% (p=0.008 n=5+5) ``` Fixes #72499. Release note: None --- .../catalog/descs/uncommitted_descriptors.go | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/pkg/sql/catalog/descs/uncommitted_descriptors.go b/pkg/sql/catalog/descs/uncommitted_descriptors.go index d353071a9c3f..8c54dc4493eb 100644 --- a/pkg/sql/catalog/descs/uncommitted_descriptors.go +++ b/pkg/sql/catalog/descs/uncommitted_descriptors.go @@ -11,6 +11,7 @@ package descs import ( + "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" @@ -100,11 +101,16 @@ type uncommittedDescriptors struct { // // TODO(postamar): better uncommitted namespace changes handling after 22.1. descNames nstree.Set + + // addedSystemDatabase is used to mark whether the optimization to add the + // system database to the set of uncommitted descriptors has occurred. + addedSystemDatabase bool } func (ud *uncommittedDescriptors) reset() { ud.descs.Clear() ud.descNames.Clear() + ud.addedSystemDatabase = false } // add adds a descriptor to the set of uncommitted descriptors and returns @@ -124,7 +130,9 @@ 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() + if id == keys.SystemDatabaseID { + ud.maybeAddSystemDatabase() + } entry := ud.descs.GetByID(id) if entry == nil { return nil, errors.NewAssertionErrorWithWrappedErrf( @@ -182,7 +190,9 @@ func maybeRefreshCachedFieldsOnTypeDescriptor( // getByID looks up an uncommitted descriptor by ID. func (ud *uncommittedDescriptors) getByID(id descpb.ID) catalog.Descriptor { - ud.maybeInitialize() + if id == keys.SystemDatabaseID && !ud.addedSystemDatabase { + ud.maybeAddSystemDatabase() + } entry := ud.descs.GetByID(id) if entry == nil { return nil @@ -200,7 +210,9 @@ 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() + if dbID == 0 && schemaID == 0 && name == systemschema.SystemDatabaseName { + ud.maybeAddSystemDatabase() + } // 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 { @@ -216,7 +228,6 @@ 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 == nil || mut.IsNew() || !mut.IsUncommittedVersion() { @@ -229,7 +240,6 @@ 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) }) @@ -286,8 +296,9 @@ var systemUncommittedDatabase = &uncommittedDescriptor{ // value lazily when this is needed, which ought to be exceedingly rare. } -func (ud *uncommittedDescriptors) maybeInitialize() { - if ud.descs.Len() == 0 { +func (ud *uncommittedDescriptors) maybeAddSystemDatabase() { + if !ud.addedSystemDatabase { + ud.addedSystemDatabase = true ud.descs.Upsert(systemUncommittedDatabase) } } From fdc1c8847b6c4e2c42159a19cbc66601f84f54dc Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Mon, 15 Nov 2021 00:50:23 -0500 Subject: [PATCH 4/5] sql/catalog/descs,nstree: fix another perf problem Release note: None --- pkg/sql/catalog/descs/uncommitted_descriptors.go | 4 ++++ pkg/sql/catalog/nstree/set.go | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/pkg/sql/catalog/descs/uncommitted_descriptors.go b/pkg/sql/catalog/descs/uncommitted_descriptors.go index 8c54dc4493eb..337626774ad1 100644 --- a/pkg/sql/catalog/descs/uncommitted_descriptors.go +++ b/pkg/sql/catalog/descs/uncommitted_descriptors.go @@ -218,6 +218,10 @@ func (ud *uncommittedDescriptors) getByName( if got := ud.descs.GetByName(dbID, schemaID, name); got != nil { return false, got.(*uncommittedDescriptor).immutable } + // Check whether the set is empty to avoid allocating the NameInfo. + if ud.descNames.Empty() { + return false, nil + } return ud.descNames.Contains(descpb.NameInfo{ ParentID: dbID, ParentSchemaID: schemaID, diff --git a/pkg/sql/catalog/nstree/set.go b/pkg/sql/catalog/nstree/set.go index 06592e8e87b2..d0bab68ada65 100644 --- a/pkg/sql/catalog/nstree/set.go +++ b/pkg/sql/catalog/nstree/set.go @@ -47,6 +47,11 @@ func (s *Set) Clear() { *s = Set{} } +// Empty returns true if the set has no entries. +func (s *Set) Empty() bool { + return !s.initialized() || s.t.Len() == 0 +} + func (s *Set) maybeInitialize() { if s.initialized() { return From 322aeaf11483da398f1aa37abaa4a790ec16c780 Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Mon, 15 Nov 2021 09:56:05 -0500 Subject: [PATCH 5/5] sql/catalog/lease: decrease lock contention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This committ takes two steps to decrease lock contention. Firstly, it makes the name cache data structure concurrent for readers with a RWLock. This goes a pretty long way to diminish contention on the acquisition path. The second change is to reduce the contention footprint when mucking with the refcounts by doing less work in the common case. Benchmark `kv95/nodes=1/cpu=32` delta over previous commit: ``` name old ops/s new ops/s delta KV95-throughput 96.4k ± 0% 99.6k ± 1% +3.36% (p=0.008 n=5+5) name old ms/s new ms/s delta KV95-P50 1.40 ± 0% 1.40 ± 0% ~ (all equal) KV95-Avg 0.50 ± 0% 0.50 ± 0% ~ (all equal) ``` Release note: None --- pkg/sql/catalog/lease/descriptor_state.go | 40 ++++++++++++------- .../catalog/lease/descriptor_version_state.go | 12 +++--- pkg/sql/catalog/lease/lease.go | 6 +-- pkg/sql/catalog/lease/name_cache.go | 10 +++-- 4 files changed, 40 insertions(+), 28 deletions(-) diff --git a/pkg/sql/catalog/lease/descriptor_state.go b/pkg/sql/catalog/lease/descriptor_state.go index a6c75aa7b7c3..75112018c413 100644 --- a/pkg/sql/catalog/lease/descriptor_state.go +++ b/pkg/sql/catalog/lease/descriptor_state.go @@ -94,6 +94,7 @@ type descriptorState struct { func (t *descriptorState) findForTimestamp( ctx context.Context, timestamp hlc.Timestamp, ) (*descriptorVersionState, bool, error) { + expensiveLogEnabled := log.ExpensiveLogEnabled(ctx, 2) t.mu.Lock() defer t.mu.Unlock() @@ -109,7 +110,7 @@ func (t *descriptorState) findForTimestamp( latest := i+1 == len(t.mu.active.data) if !desc.hasExpired(timestamp) { // Existing valid descriptor version. - desc.incRefCount(ctx) + desc.incRefCount(ctx, expensiveLogEnabled) return desc, latest, nil } @@ -220,29 +221,37 @@ func (t *descriptorState) release(ctx context.Context, s *descriptorVersionState // Decrements the refcount and returns true if the lease has to be removed // from the store. - decRefcount := func(s *descriptorVersionState) *storedLease { + expensiveLoggingEnabled := log.ExpensiveLogEnabled(ctx, 2) + decRefCount := func(s *descriptorVersionState) (shouldRemove bool) { + s.mu.Lock() + defer s.mu.Unlock() + s.mu.refcount-- + if expensiveLoggingEnabled { + log.Infof(ctx, "release: %s", s.stringLocked()) + } + return s.mu.refcount == 0 + } + maybeMarkRemoveStoredLease := func(s *descriptorVersionState) *storedLease { // Figure out if we'd like to remove the lease from the store asap (i.e. // when the refcount drops to 0). If so, we'll need to mark the lease as // invalid. - removeOnceDereferenced := t.m.removeOnceDereferenced() || + removeOnceDereferenced := // Release from the store if the descriptor has been dropped or taken // offline. t.mu.takenOffline || - // Release from the store if the lease is not for the latest - // version; only leases for the latest version can be acquired. - s != t.mu.active.findNewest() || - s.GetVersion() < t.mu.maxVersionSeen - + // Release from the store if the lease is not for the latest + // version; only leases for the latest version can be acquired. + s != t.mu.active.findNewest() || + s.GetVersion() < t.mu.maxVersionSeen || + t.m.removeOnceDereferenced() + if !removeOnceDereferenced { + return nil + } s.mu.Lock() defer s.mu.Unlock() - s.mu.refcount-- - if log.ExpensiveLogEnabled(ctx, 2) { - log.Infof(ctx, "release: %s", s.stringLocked()) - } if s.mu.refcount < 0 { panic(errors.AssertionFailedf("negative ref count: %s", s)) } - if s.mu.refcount == 0 && s.mu.lease != nil && removeOnceDereferenced { l := s.mu.lease s.mu.lease = nil @@ -251,9 +260,12 @@ func (t *descriptorState) release(ctx context.Context, s *descriptorVersionState return nil } maybeRemoveLease := func() *storedLease { + if shouldRemove := decRefCount(s); !shouldRemove { + return nil + } t.mu.Lock() defer t.mu.Unlock() - if l := decRefcount(s); l != nil { + if l := maybeMarkRemoveStoredLease(s); l != nil { t.mu.active.remove(s) return l } diff --git a/pkg/sql/catalog/lease/descriptor_version_state.go b/pkg/sql/catalog/lease/descriptor_version_state.go index eed9b57810d2..062e682db347 100644 --- a/pkg/sql/catalog/lease/descriptor_version_state.go +++ b/pkg/sql/catalog/lease/descriptor_version_state.go @@ -77,9 +77,7 @@ func (s *descriptorVersionState) Underlying() catalog.Descriptor { } func (s *descriptorVersionState) Expiration() hlc.Timestamp { - s.mu.Lock() - defer s.mu.Unlock() - return s.mu.expiration + return s.getExpiration() } func (s *descriptorVersionState) SafeMessage() string { @@ -113,15 +111,15 @@ func (s *descriptorVersionState) hasExpiredLocked(timestamp hlc.Timestamp) bool return s.mu.expiration.LessEq(timestamp) } -func (s *descriptorVersionState) incRefCount(ctx context.Context) { +func (s *descriptorVersionState) incRefCount(ctx context.Context, expensiveLogEnabled bool) { s.mu.Lock() defer s.mu.Unlock() - s.incRefCountLocked(ctx) + s.incRefCountLocked(ctx, expensiveLogEnabled) } -func (s *descriptorVersionState) incRefCountLocked(ctx context.Context) { +func (s *descriptorVersionState) incRefCountLocked(ctx context.Context, expensiveLogEnabled bool) { s.mu.refcount++ - if log.ExpensiveLogEnabled(ctx, 2) { + if expensiveLogEnabled { log.VEventf(ctx, 2, "descriptorVersionState.incRefCount: %s", s.stringLocked()) } } diff --git a/pkg/sql/catalog/lease/lease.go b/pkg/sql/catalog/lease/lease.go index 31134e1c45d4..809ad1ecf8b4 100644 --- a/pkg/sql/catalog/lease/lease.go +++ b/pkg/sql/catalog/lease/lease.go @@ -666,9 +666,9 @@ func NewLeaseManager( func NameMatchesDescriptor( desc catalog.Descriptor, parentID descpb.ID, parentSchemaID descpb.ID, name string, ) bool { - return desc.GetParentID() == parentID && - desc.GetParentSchemaID() == parentSchemaID && - desc.GetName() == name + return desc.GetName() == name && + desc.GetParentID() == parentID && + desc.GetParentSchemaID() == parentSchemaID } // findNewest returns the newest descriptor version state for the ID. diff --git a/pkg/sql/catalog/lease/name_cache.go b/pkg/sql/catalog/lease/name_cache.go index a65a26960d2f..ef676ff37481 100644 --- a/pkg/sql/catalog/lease/name_cache.go +++ b/pkg/sql/catalog/lease/name_cache.go @@ -16,6 +16,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/nstree" "github.com/cockroachdb/cockroach/pkg/util/hlc" + "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/syncutil" "github.com/cockroachdb/errors" ) @@ -29,7 +30,7 @@ func makeNameCache() nameCache { // from the store. The cache maintains the latest version for each name. // All methods are thread-safe. type nameCache struct { - mu syncutil.Mutex + mu syncutil.RWMutex descriptors nstree.Map } @@ -47,14 +48,15 @@ func (c *nameCache) get( name string, timestamp hlc.Timestamp, ) *descriptorVersionState { - c.mu.Lock() + c.mu.RLock() desc, ok := c.descriptors.GetByName( parentID, parentSchemaID, name, ).(*descriptorVersionState) - c.mu.Unlock() + c.mu.RUnlock() if !ok { return nil } + expensiveLogEnabled := log.ExpensiveLogEnabled(ctx, 2) desc.mu.Lock() if desc.mu.lease == nil { desc.mu.Unlock() @@ -80,7 +82,7 @@ func (c *nameCache) get( return nil } - desc.incRefCountLocked(ctx) + desc.incRefCountLocked(ctx, expensiveLogEnabled) return desc }