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.

Sign up for GitHub

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

Jump to bottom

catalog/lease: reduce lock contention on descVersionState #111397

Merged
merged 1 commit into from
Oct 2, 2023
Merged

catalog/lease: reduce lock contention on descVersionState #111397

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions pkg/sql/catalog/lease/lease.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
20 changes: 10 additions & 10 deletions pkg/sql/catalog/lease/lease_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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(),
Expand All @@ -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(),
Expand Down Expand Up @@ -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(),
Expand All @@ -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)
}
Expand Down Expand Up @@ -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")
}
}
Expand Down
20 changes: 11 additions & 9 deletions pkg/sql/catalog/lease/name_cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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()
Copy link
Collaborator

Choose a reason for hiding this comment

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

as described in As described in #109443 (comment), RWMutex performs badly under high concurrency. Should we just change to a normal mutex?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me try this out and see how it behaves without this change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it's similar to the old one:
BenchmarkLeaseAcquireByNameCached-24 1624814 734.4 ns/op

Also, there are no writes to the name cache in this scenario, so it should hit the fast path on the RWMutex (outside of system table activity).

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()
Expand All @@ -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()
Expand All @@ -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) {
Expand Down