Skip to content

Commit b76e68c

Browse files
committed
catalog/lease: fix queries against initial versions in locked mode
Previously, all of our testing for locked timestamping included WaitForInitialVersion being enabled. This configuration ensured that we would always acquire the first version of leased descriptors in our tests. However, upon enabling locked leasing across the binary, we encountered issues because bootstrapped tables and other tests might not secure the initial version checkout. Consequently, we would incorrectly declare the object as non-existent if the lease timestamp precoded the object's creation. Fixes: #156401 Release note: None
1 parent c6539d4 commit b76e68c

File tree

3 files changed

+30
-1
lines changed

3 files changed

+30
-1
lines changed

pkg/sql/catalog/lease/descriptor_set.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,13 @@ func (l *descriptorSet) findIndex(version descpb.DescriptorVersion) (int, bool)
8484
return i, false
8585
}
8686

87+
func (l *descriptorSet) findOldest() *descriptorVersionState {
88+
if len(l.data) == 0 {
89+
return nil
90+
}
91+
return l.data[0]
92+
}
93+
8794
func (l *descriptorSet) findNewest() *descriptorVersionState {
8895
if len(l.data) == 0 {
8996
return nil

pkg/sql/catalog/lease/descriptor_state.go

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,9 @@ func (t *descriptorState) findForTimestampImpl(
105105
readTimestamp hlc.Timestamp,
106106
expensiveLogEnabled bool,
107107
) (*descriptorVersionState, bool, error) {
108+
// Normally this is true if an older timestamp is intentionally used for
109+
// locked leasing.
110+
hasDifferentReadTimeStamp := leaseTimestamp != readTimestamp
108111
// Walk back the versions to find one that is valid for the timestamp.
109112
for i := len(t.mu.active.data) - 1; i >= 0; i-- {
110113
// Check to see if the ModificationTime is valid. If only the initial version
@@ -115,7 +118,7 @@ func (t *descriptorState) findForTimestampImpl(
115118
// Existing valid descriptor version.
116119
desc.incRefCount(ctx, expensiveLogEnabled)
117120
return desc, latest, nil
118-
} else if !latest && readTimestamp != leaseTimestamp {
121+
} else if !latest && hasDifferentReadTimeStamp {
119122
// The lease timestamp is not compatible with the read timestamp, since
120123
// the descriptor returned will be expired. This means we are seeing the
121124
// first read of this descriptor, since the prior version was not locked.
@@ -133,6 +136,16 @@ func (t *descriptorState) findForTimestampImpl(
133136
}
134137
}
135138

139+
// If we have the initial version of the descriptor, and it satisfies the read
140+
// timestamp, then the object was just created. We can confirm it satisfies
141+
// the request, by executing findForTimestampImpl with the readTimestamp instead.
142+
if oldest := t.mu.active.findOldest(); hasDifferentReadTimeStamp &&
143+
oldest != nil &&
144+
oldest.GetVersion() == 1 &&
145+
oldest.GetModificationTime().LessEq(readTimestamp) {
146+
return t.findForTimestampImpl(ctx, readTimestamp, readTimestamp, expensiveLogEnabled)
147+
}
148+
136149
return nil, false, errReadOlderVersion
137150
}
138151

pkg/sql/catalog/lease/lease_internal_test.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1747,6 +1747,10 @@ func TestLeaseManagerLockedTimestampBasic(t *testing.T) {
17471747
st := cluster.MakeTestingClusterSettings()
17481748
ctx := context.Background()
17491749
LockedLeaseTimestamp.Override(ctx, &st.SV, true)
1750+
// Intentionally disable WaitForInitialVersion support, so that we can run
1751+
// historical queries at timestamps before the lease manager is fully caught
1752+
// up.
1753+
WaitForInitialVersion.Override(ctx, &st.SV, false)
17501754
srv, db, _ := serverutils.StartServer(
17511755
t, base.TestServerArgs{
17521756
// Avoid using tenants since async tenant migration steps can acquire
@@ -1770,6 +1774,11 @@ func TestLeaseManagerLockedTimestampBasic(t *testing.T) {
17701774
r := sqlutils.MakeSQLRunner(db)
17711775

17721776
r.Exec(t, "CREATE TABLE t1(n int)")
1777+
// These queries will be intentionally executed before the lease manager is
1778+
// fully aware of their existence, since WaitForInitialVersion support is
1779+
// disabled. We should be forced to use a non-locked timestamp to be aware
1780+
// of the tables existence.
1781+
r.Exec(t, "SELECT * FROM t1")
17731782
r.Exec(t, "INSERT INTO t1 VALUES (1)")
17741783

17751784
grp := ctxgroup.WithContext(context.Background())

0 commit comments

Comments
 (0)