From a177cd4db0b55fed266bec84ddc67c93534258d4 Mon Sep 17 00:00:00 2001 From: Bergin Dedej Date: Tue, 17 Dec 2024 17:25:45 +0000 Subject: [PATCH] sql: TestReacquireLeaseOnRestart fix Previously we had an extra condition in an if statement which caused this failure. This was the condition `errors.Is(err, errRenewLease)` it is now removed and we now set toRelease to nil when `session != nil` Fixes: #129421 Release note: none --- pkg/sql/catalog/lease/descriptor_state.go | 4 ++++ pkg/sql/catalog/lease/lease.go | 10 +--------- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/pkg/sql/catalog/lease/descriptor_state.go b/pkg/sql/catalog/lease/descriptor_state.go index ed1dc290a691..689680f2d640 100644 --- a/pkg/sql/catalog/lease/descriptor_state.go +++ b/pkg/sql/catalog/lease/descriptor_state.go @@ -172,6 +172,10 @@ func (t *descriptorState) upsertLeaseLocked( } if session != nil { s.mu.lease.sessionID = session.ID().UnsafeBytes() + // When using session based leasing, if we end up acquiring the same lease again + // nothing needs to be cleaned up or updated. This is because the system.lease + // table does not store any expiry inside the table. + toRelease.sessionID = nil } if log.ExpensiveLogEnabled(ctx, 2) { log.VEventf(ctx, 2, "replaced lease: %s with %s", toRelease, s.mu.lease) diff --git a/pkg/sql/catalog/lease/lease.go b/pkg/sql/catalog/lease/lease.go index f30ea8b213aa..7a7fa3c21a87 100644 --- a/pkg/sql/catalog/lease/lease.go +++ b/pkg/sql/catalog/lease/lease.go @@ -812,16 +812,8 @@ func purgeOldVersions( // Acquire a refcount on the descriptor on the latest version to maintain an // active lease, so that it doesn't get released when removeInactives() // is called below. Release this lease after calling removeInactives(). - // - // If the lease ends up being expired anyway it's okay to purge all previous - // versions (assuming no one has a ref count). With the session based - // leasing upgrade and tests with zero lease duration, we can have the lease - // expire right after acquiring it. Because renewals are disabled in later - // stages of this migration there is no other mechanism to purge old versions - // if we hit this case. Note: This scenario is impossible to hit in the real - // world since the lease duration is never set to 0. desc, _, err := t.findForTimestamp(ctx, m.storage.clock.Now()) - if isInactive := catalog.HasInactiveDescriptorError(err); err == nil || isInactive || errors.Is(err, errRenewLease) { + if isInactive := catalog.HasInactiveDescriptorError(err); err == nil || isInactive { removeInactives(isInactive) if desc != nil { t.release(ctx, desc)