catalog/lease: reduce lock contention on descVersionState

Recently, a regression was noticed on master related to AcquireByName
being slower between builds. Using a profiler, this was tracked down to
contention on descriptorVersionState's mutex.  To address this, this
patch avoids acquiring this lock twice in the hot path, which resolves
the regression.

After:   BenchmarkLeaseAcquireByNameCached-24    	 2378610	       451.9 ns/op
Before:  BenchmarkLeaseAcquireByNameCached-24    	 1493899	       727.9 ns/op
f13c021: BenchmarkLeaseAcquireByNameCached-24    	 2129446	       568.1 ns/op

Fixes: cockroachdb#111094

Release note: None

pkg/sql/catalog/lease/lease.go

@@ -840,10 +840,9 @@ func (m *Manager) AcquireByName(
 		return desc, nil
 	}
 	// Check if we have cached an ID for this name.
-	descVersion := m.names.get(ctx, parentID, parentSchemaID, name, timestamp)
+	descVersion, expiration := m.names.get(ctx, parentID, parentSchemaID, name, timestamp)
 	if descVersion != nil {
 		if descVersion.GetModificationTime().LessEq(timestamp) {
-			expiration := descVersion.getExpiration()
 			// If this lease is nearly expired, ensure a renewal is queued.
 			durationUntilExpiry := time.Duration(expiration.WallTime - timestamp.WallTime)
 			if durationUntilExpiry < m.storage.leaseRenewalTimeout() {

pkg/sql/catalog/lease/lease_internal_test.go

@@ -396,7 +396,7 @@ CREATE TEMP TABLE t2 (temp int);
 
 	for _, tableName := range []string{"t", "t2"} {
 		tableDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, s.Codec(), "defaultdb", tableName)
-		lease := leaseManager.names.get(
+		lease, _ := leaseManager.names.get(
 			context.Background(),
 			tableDesc.GetParentID(),
 			tableDesc.GetParentSchemaID(),
@@ -440,17 +440,17 @@ CREATE TABLE t.test (k CHAR PRIMARY KEY, v CHAR);
 	}
 
 	// Check that the cache has been updated.
-	if leaseManager.names.get(
+	if lease, _ := leaseManager.names.get(
 		context.Background(),
 		tableDesc.GetParentID(),
 		tableDesc.GetParentSchemaID(),
 		"test",
 		s.Clock().Now(),
-	) != nil {
+	); lease != nil {
 		t.Fatalf("old name still in cache")
 	}
 
-	lease := leaseManager.names.get(
+	lease, _ := leaseManager.names.get(
 		context.Background(),
 		tableDesc.GetParentID(),
 		tableDesc.GetParentSchemaID(),
@@ -494,7 +494,7 @@ CREATE TABLE t.%s (k CHAR PRIMARY KEY, v CHAR);
 
 	// Check the assumptions this tests makes: that there is a cache entry
 	// (with a valid lease).
-	if lease := leaseManager.names.get(
+	if lease, _ := leaseManager.names.get(
 		context.Background(),
 		tableDesc.GetParentID(),
 		tableDesc.GetParentSchemaID(),
@@ -509,7 +509,7 @@ CREATE TABLE t.%s (k CHAR PRIMARY KEY, v CHAR);
 	leaseManager.ExpireLeases(s.Clock())
 
 	// Check the name no longer resolves.
-	if lease := leaseManager.names.get(
+	if lease, _ := leaseManager.names.get(
 		context.Background(),
 		tableDesc.GetParentID(),
 		tableDesc.GetParentSchemaID(),
@@ -556,7 +556,7 @@ CREATE TABLE t.%s (k CHAR PRIMARY KEY, v CHAR);
 	}
 
 	// There is a cache entry.
-	lease := leaseManager.names.get(
+	lease, _ := leaseManager.names.get(
 		context.Background(),
 		tableDesc.GetParentID(),
 		tableDesc.GetParentSchemaID(),
@@ -577,7 +577,7 @@ CREATE TABLE t.%s (k CHAR PRIMARY KEY, v CHAR);
 	}
 
 	// Check the name resolves to the new lease.
-	newLease := leaseManager.names.get(context.Background(), tableDesc.GetParentID(), tableDesc.GetParentSchemaID(), tableName, s.Clock().Now())
+	newLease, _ := leaseManager.names.get(context.Background(), tableDesc.GetParentID(), tableDesc.GetParentSchemaID(), tableName, s.Clock().Now())
 	if newLease == nil {
 		t.Fatalf("name cache doesn't contain entry for (%d, %s)", tableDesc.GetParentID(), tableName)
 	}
@@ -621,13 +621,13 @@ CREATE TABLE t.test (k CHAR PRIMARY KEY, v CHAR);
 	tableDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, s.Codec(), "t", "test")
 
 	// Check that we cannot get the table by a different name.
-	if leaseManager.names.get(
+	if lease, _ := leaseManager.names.get(
 		context.Background(),
 		tableDesc.GetParentID(),
 		tableDesc.GetParentSchemaID(),
 		"tEsT",
 		s.Clock().Now(),
-	) != nil {
+	); lease != nil {
 		t.Fatalf("lease manager incorrectly found table with different case")
 	}
 }

pkg/sql/catalog/lease/name_cache.go

@@ -35,9 +35,10 @@ type nameCache struct {
 }
 
 // Resolves a (qualified) name to the descriptor's ID.
-// Returns a valid descriptorVersionState for descriptor with that name,
-// if the name had been previously cached and the cache has a descriptor
-// version that has not expired. Returns nil otherwise.
+// Returns a valid descriptorVersionState and expiration (hlc.Timestamp)
+// for descriptor with that name, if the name had been previously cached
+// and the cache has a descriptor version that has not expired.
+// Returns nil (and empty timestamp) otherwise.
 // This method handles normalizing the descriptor name.
 // The descriptor's refcount is incremented before returning, so the caller
 // is responsible for releasing it to the leaseManager.
@@ -47,14 +48,15 @@ func (c *nameCache) get(
 	parentSchemaID descpb.ID,
 	name string,
 	timestamp hlc.Timestamp,
-) *descriptorVersionState {
+) (desc *descriptorVersionState, expiration hlc.Timestamp) {
 	c.mu.RLock()
-	desc, ok := c.descriptors.GetByName(
+	var ok bool
+	desc, ok = c.descriptors.GetByName(
 		parentID, parentSchemaID, name,
 	).(*descriptorVersionState)
 	c.mu.RUnlock()
 	if !ok {
-		return nil
+		return nil, expiration
 	}
 	expensiveLogEnabled := log.ExpensiveLogEnabled(ctx, 2)
 	desc.mu.Lock()
@@ -63,7 +65,7 @@ func (c *nameCache) get(
 		// This get() raced with a release operation. Remove this cache
 		// entry if needed.
 		c.remove(desc)
-		return nil
+		return nil, hlc.Timestamp{}
 	}
 
 	defer desc.mu.Unlock()
@@ -79,11 +81,11 @@ func (c *nameCache) get(
 
 	// Expired descriptor. Don't hand it out.
 	if desc.hasExpiredLocked(timestamp) {
-		return nil
+		return nil, hlc.Timestamp{}
 	}
 
 	desc.incRefCountLocked(ctx, expensiveLogEnabled)
-	return desc
+	return desc, desc.mu.expiration
 }
 
 func (c *nameCache) insert(desc *descriptorVersionState) {