Merge #118329 #118800 #118923

118329: kvclient: misc range cache cleanups r=arulajmani a=andrewbaptist

Numerous small cleanups to the range cache. 

After these changes, The RangeCache and related classes have a much simpler interface with some nice usability benefits:

1) CacheEntry is no longer public. This is good because it is intended to be immutable and it is now easier to audit its usage.
2) Test only methods are more clearly changed to designate them that way. This will prevent new code from using them incorrectly
3) Unused methods are removed from the interface.

118800: *: use new slice to array conversion syntax r=nvanbenschoten a=nvanbenschoten

Closes #110261.

This commit adopts the new slice to array conversion syntax introduced in go 1.20. The new syntax is only applicable in one small part of the codebase, so it's a small change.

From https://tip.golang.org/doc/go1.20:

> Go 1.20 extends this to allow conversions from a slice to an array: given a slice x, [4]byte(x) can now be written instead of *(*[4]byte)(x).

Release note: None

118923: sql: reduce allocations in rowContainerIterator r=mgartner a=mgartner

In #111318 a shallow copy of rows was added to
`rowContainerIterator.Next`. This copy is only necessary for one usage
of the iterator within `plpgsqlCursorHelper`. This commit moves the copy
to the cursor helper which reduces allocations for all other usages of
the iterator.

Informs #117546

Release note: None


Co-authored-by: Andrew Baptist <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>

Author: 4 people

pkg/ccl/kvccl/kvfollowerreadsccl/followerreads_test.go

@@ -814,14 +814,14 @@ func TestFollowerReadsWithStaleDescriptor(t *testing.T) {
 	n1.QueryRow(t, `SELECT id from system.namespace WHERE name='test'`).Scan(&tableID)
 	tablePrefix := keys.MustAddr(keys.SystemSQLCodec.TablePrefix(tableID))
 	n4Cache := tc.Server(3).DistSenderI().(*kvcoord.DistSender).RangeDescriptorCache()
-	entry := n4Cache.GetCached(ctx, tablePrefix, false /* inverted */)
-	require.NotNil(t, entry)
-	require.False(t, entry.Lease().Empty())
-	require.Equal(t, roachpb.StoreID(1), entry.Lease().Replica.StoreID)
+	entry, err := n4Cache.TestingGetCached(ctx, tablePrefix, false /* inverted */)
+	require.NoError(t, err)
+	require.False(t, entry.Lease.Empty())
+	require.Equal(t, roachpb.StoreID(1), entry.Lease.Replica.StoreID)
 	require.Equal(t, []roachpb.ReplicaDescriptor{
 		{NodeID: 1, StoreID: 1, ReplicaID: 1},
 		{NodeID: 2, StoreID: 2, ReplicaID: 2},
-	}, entry.Desc().Replicas().Descriptors())
+	}, entry.Desc.Replicas().Descriptors())
 
 	// Remove the follower and add a new non-voter to n3. n2 will no longer have a
 	// replica.
@@ -837,19 +837,19 @@ func TestFollowerReadsWithStaleDescriptor(t *testing.T) {
 	rec := <-recCh
 	require.False(t, kv.OnlyFollowerReads(rec), "query was served through follower reads: %s", rec)
 	// Check that the cache was properly updated.
-	entry = n4Cache.GetCached(ctx, tablePrefix, false /* inverted */)
-	require.NotNil(t, entry)
-	require.False(t, entry.Lease().Empty())
-	require.Equal(t, roachpb.StoreID(1), entry.Lease().Replica.StoreID)
+	entry, err = n4Cache.TestingGetCached(ctx, tablePrefix, false /* inverted */)
+	require.NoError(t, err)
+	require.False(t, entry.Lease.Empty())
+	require.Equal(t, roachpb.StoreID(1), entry.Lease.Replica.StoreID)
 	require.Equal(t, []roachpb.ReplicaDescriptor{
 		{NodeID: 1, StoreID: 1, ReplicaID: 1},
 		{NodeID: 3, StoreID: 3, ReplicaID: 3, Type: roachpb.NON_VOTER},
-	}, entry.Desc().Replicas().Descriptors())
+	}, entry.Desc.Replicas().Descriptors())
 
 	// Make a note of the follower reads metric on n3. We'll check that it was
 	// incremented.
 	var followerReadsCountBefore int64
-	err := tc.Servers[2].GetStores().(*kvserver.Stores).VisitStores(func(s *kvserver.Store) error {
+	err = tc.Servers[2].GetStores().(*kvserver.Stores).VisitStores(func(s *kvserver.Store) error {
 		followerReadsCountBefore = s.Metrics().FollowerReadsCount.Count()
 		return nil
 	})
@@ -880,14 +880,14 @@ func TestFollowerReadsWithStaleDescriptor(t *testing.T) {
 	n3 := sqlutils.MakeSQLRunner(tc.Conns[2])
 	n3.Exec(t, "SELECT * from test WHERE k=1")
 	n3Cache := tc.Server(2).DistSenderI().(*kvcoord.DistSender).RangeDescriptorCache()
-	entry = n3Cache.GetCached(ctx, tablePrefix, false /* inverted */)
-	require.NotNil(t, entry)
-	require.False(t, entry.Lease().Empty())
-	require.Equal(t, roachpb.StoreID(1), entry.Lease().Replica.StoreID)
+	entry, err = n3Cache.TestingGetCached(ctx, tablePrefix, false /* inverted */)
+	require.NoError(t, err)
+	require.False(t, entry.Lease.Empty())
+	require.Equal(t, roachpb.StoreID(1), entry.Lease.Replica.StoreID)
 	require.Equal(t, []roachpb.ReplicaDescriptor{
 		{NodeID: 1, StoreID: 1, ReplicaID: 1},
 		{NodeID: 3, StoreID: 3, ReplicaID: 3, Type: roachpb.NON_VOTER},
-	}, entry.Desc().Replicas().Descriptors())
+	}, entry.Desc.Replicas().Descriptors())
 
 	// Enable DistSQL so that we have a distributed plan with a single flow on
 	// n3 (local plans ignore the misplanned ranges).
@@ -1129,15 +1129,15 @@ func TestSecondaryTenantFollowerReadsRouting(t *testing.T) {
 			tenantSQL.Exec(t, `SELECT * FROM t.test WHERE k = 1`)
 			tablePrefix := keys.MustAddr(codec.TenantPrefix())
 			cache := tenants[gatewayNode].DistSenderI().(*kvcoord.DistSender).RangeDescriptorCache()
-			entry := cache.GetCached(ctx, tablePrefix, false /* inverted */)
-			require.NotNil(t, entry)
-			require.False(t, entry.Lease().Empty())
-			require.Equal(t, roachpb.StoreID(1), entry.Lease().Replica.StoreID)
+			entry, err := cache.TestingGetCached(ctx, tablePrefix, false /* inverted */)
+			require.NoError(t, err)
+			require.False(t, entry.Lease.Empty())
+			require.Equal(t, roachpb.StoreID(1), entry.Lease.Replica.StoreID)
 			require.Equal(t, []roachpb.ReplicaDescriptor{
 				{NodeID: 1, StoreID: 1, ReplicaID: 1},
 				{NodeID: 2, StoreID: 2, ReplicaID: 2},
 				{NodeID: 3, StoreID: 3, ReplicaID: 3},
-			}, entry.Desc().Replicas().Descriptors())
+			}, entry.Desc.Replicas().Descriptors())
 
 			tenantSQL.Exec(t, historicalQuery)
 			rec := <-recCh

pkg/ccl/multiregionccl/datadriven_test.go

@@ -319,11 +319,11 @@ SET CLUSTER SETTING kv.allocator.min_lease_transfer_interval = '5m'
 				}
 				cache := ds.tc.Server(idx).DistSenderI().(*kvcoord.DistSender).RangeDescriptorCache()
 				tablePrefix := keys.MustAddr(keys.SystemSQLCodec.TablePrefix(tableID))
-				entry := cache.GetCached(ctx, tablePrefix, false /* inverted */)
-				if entry == nil {
-					return errors.Newf("no entry found for %s in cache", tbName).Error()
+				entry, err := cache.TestingGetCached(ctx, tablePrefix, false /* inverted */)
+				if err != nil {
+					return err.Error()
 				}
-				return entry.ClosedTimestampPolicy().String()
+				return entry.ClosedTimestampPolicy.String()
 
 			case "wait-for-zone-config-changes":
 				lookupKey, err := getRangeKeyForInput(t, d, ds.tc)

pkg/ccl/multiregionccl/roundtrips_test.go

@@ -123,15 +123,15 @@ func TestEnsureLocalReadsOnGlobalTables(t *testing.T) {
 
 			// Check that the cache was indeed populated.
 			cache := tc.Server(i).DistSenderI().(*kvcoord.DistSender).RangeDescriptorCache()
-			entry := cache.GetCached(context.Background(), tablePrefix, false /* inverted */)
-			require.NotNil(t, entry.Lease().Empty())
-			require.NotNil(t, entry)
+			entry, err := cache.TestingGetCached(context.Background(), tablePrefix, false /* inverted */)
+			require.NoError(t, err)
+			require.NotNil(t, entry.Lease.Empty())
 
-			if expected, got := roachpb.LEAD_FOR_GLOBAL_READS, entry.ClosedTimestampPolicy(); got != expected {
+			if expected, got := roachpb.LEAD_FOR_GLOBAL_READS, entry.ClosedTimestampPolicy; got != expected {
 				return errors.Newf("expected closedts policy %s, got %s", expected, got)
 			}
 
-			isLeaseHolder = entry.Lease().Replica.NodeID == tc.Server(i).NodeID()
+			isLeaseHolder = entry.Lease.Replica.NodeID == tc.Server(i).NodeID()
 			return nil
 		})
 

pkg/kv/bulk/sst_batcher.go

@@ -449,7 +449,7 @@ func (b *SSTBatcher) flushIfNeeded(ctx context.Context, nextKey roachpb.Key) err
 			if r, err := b.rc.Lookup(ctx, k); err != nil {
 				log.Warningf(ctx, "failed to lookup range cache entry for key %v: %v", k, err)
 			} else {
-				k := r.Desc().EndKey.AsRawKey()
+				k := r.Desc.EndKey.AsRawKey()
 				b.flushKey = k
 				log.VEventf(ctx, 3, "%s building sstable that will flush before %v", b.name, k)
 			}

pkg/kv/bulk/sst_batcher_test.go

@@ -294,7 +294,7 @@ func runTestImport(t *testing.T, batchSizeValue int64) {
 			for _, k := range []int{0, split1} {
 				ent, err := ds.RangeDescriptorCache().Lookup(ctx, keys.MustAddr(key(k)))
 				require.NoError(t, err)
-				mockCache.Insert(ctx, roachpb.RangeInfo{Desc: *ent.Desc()})
+				mockCache.Insert(ctx, ent)
 			}
 
 			t.Logf("splitting at %s", key(split2))

pkg/kv/kvclient/kvcoord/dist_sender_test.go

@@ -850,16 +850,14 @@ func TestRetryOnNotLeaseHolderError(t *testing.T) {
 				t.Fatalf("unexpected error: %v", pErr)
 			}
 			require.Equal(t, 2, attempts)
-			rng := ds.rangeCache.GetCached(ctx, testUserRangeDescriptor.StartKey, false /* inverted */)
-			require.NotNil(t, rng)
+			rng, err := ds.rangeCache.TestingGetCached(ctx, testUserRangeDescriptor.StartKey, false /* inverted */)
+			require.NoError(t, err)
 
 			if tc.expLeaseholder != nil {
-				lh := rng.Leaseholder()
-				require.NotNil(t, lh)
-				require.Equal(t, tc.expLeaseholder, lh)
+				lh := rng.Lease.Replica
+				require.Equal(t, *tc.expLeaseholder, lh)
 				if tc.expLease {
-					l := rng.Lease()
-					require.NotNil(t, l)
+					l := rng.Lease
 					require.Equal(t, *tc.expLeaseholder, l.Replica)
 					// The transport retry will use the replica descriptor from the
 					// initial range descriptor, not the one returned in the NLHE, i.e.
@@ -868,7 +866,7 @@ func TestRetryOnNotLeaseHolderError(t *testing.T) {
 					expRetryReplica.Type = 0
 					require.Equal(t, expRetryReplica, retryReplica)
 				} else {
-					require.Nil(t, rng.Lease())
+					require.True(t, rng.Lease.Empty())
 				}
 			}
 		})
@@ -1529,10 +1527,11 @@ func TestDistSenderLeaseholderDown(t *testing.T) {
 		t.Errorf("contacted n1: %t, contacted n2: %t", contacted1, contacted2)
 	}
 
-	rng := ds.rangeCache.GetCached(ctx, testUserRangeDescriptor.StartKey, false /* inverted */)
-	require.Equal(t, desc, *rng.Desc())
-	require.Equal(t, roachpb.StoreID(2), rng.Lease().Replica.StoreID)
-	require.Equal(t, roachpb.LAG_BY_CLUSTER_SETTING, rng.ClosedTimestampPolicy())
+	rng, err := ds.rangeCache.TestingGetCached(ctx, testUserRangeDescriptor.StartKey, false /* inverted */)
+	require.NoError(t, err)
+	require.Equal(t, desc, rng.Desc)
+	require.Equal(t, roachpb.StoreID(2), rng.Lease.Replica.StoreID)
+	require.Equal(t, roachpb.LAG_BY_CLUSTER_SETTING, rng.ClosedTimestampPolicy)
 }
 
 // TestRetryOnDescriptorLookupError verifies that the DistSender retries a descriptor
@@ -1793,11 +1792,11 @@ func TestEvictCacheOnError(t *testing.T) {
 			if _, pErr := kv.SendWrapped(ctx, ds, put); pErr != nil && !testutils.IsPError(pErr, errString) && !testutils.IsError(pErr.GoError(), ctx.Err().Error()) {
 				t.Errorf("put encountered unexpected error: %s", pErr)
 			}
-			rng := ds.rangeCache.GetCached(ctx, testUserRangeDescriptor.StartKey, false /* inverted */)
+			rng, err := ds.rangeCache.TestingGetCached(ctx, testUserRangeDescriptor.StartKey, false /* inverted */)
 			if tc.shouldClearReplica {
-				require.Nil(t, rng)
+				require.Error(t, err)
 			} else if tc.shouldClearLeaseHolder {
-				require.True(t, rng.Lease().Empty())
+				require.True(t, rng.Lease.Empty())
 			}
 		})
 	}
@@ -2301,16 +2300,8 @@ func TestDistSenderDescriptorUpdatesOnSuccessfulRPCs(t *testing.T) {
 			// Check that the cache has the updated descriptor returned by the RPC.
 			for _, ri := range tc {
 				rk := ri.Desc.StartKey
-				entry := ds.rangeCache.GetCached(ctx, rk, false /* inverted */)
-				require.NotNil(t, entry)
-				require.Equal(t, &ri.Desc, entry.Desc())
-				if ri.Lease.Empty() {
-					require.Nil(t, entry.Leaseholder())
-					require.Nil(t, entry.Lease())
-				} else {
-					require.Equal(t, &ri.Lease, entry.Lease())
-				}
-				require.Equal(t, ri.ClosedTimestampPolicy, entry.ClosedTimestampPolicy())
+				entry, _ := ds.rangeCache.TestingGetCached(ctx, rk, false /* inverted */)
+				require.Equal(t, ri, entry)
 			}
 		})
 	}
@@ -2374,9 +2365,10 @@ func TestSendRPCRangeNotFoundError(t *testing.T) {
 			if len(seen) == 1 {
 				// Pretend that this replica is the leaseholder in the cache to verify
 				// that the response evicts it.
-				rng := ds.rangeCache.GetCached(ctx, descriptor.StartKey, false /* inverse */)
+				rng, err := ds.rangeCache.TestingGetCached(ctx, descriptor.StartKey, false /* inverse */)
+				require.NoError(t, err)
 				ds.rangeCache.Insert(ctx, roachpb.RangeInfo{
-					Desc:  *rng.Desc(),
+					Desc:  rng.Desc,
 					Lease: roachpb.Lease{Replica: ba.Replica},
 				})
 			}
@@ -2409,9 +2401,9 @@ func TestSendRPCRangeNotFoundError(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	rng := ds.rangeCache.GetCached(ctx, descriptor.StartKey, false /* inverted */)
-	require.NotNil(t, rng)
-	require.Equal(t, leaseholderStoreID, rng.Lease().Replica.StoreID)
+	rng, e := ds.rangeCache.TestingGetCached(ctx, descriptor.StartKey, false /* inverted */)
+	require.NoError(t, e)
+	require.Equal(t, leaseholderStoreID, rng.Lease.Replica.StoreID)
 }
 
 func TestMultiRangeGapReverse(t *testing.T) {
@@ -4084,10 +4076,10 @@ func TestCanSendToFollower(t *testing.T) {
 			// we've had where we were always updating the leaseholder on successful
 			// RPCs because we erroneously assumed that a success must come from the
 			// leaseholder.
-			rng := ds.rangeCache.GetCached(ctx, testUserRangeDescriptor.StartKey, false /* inverted */)
-			require.NotNil(t, rng)
-			require.NotNil(t, rng.Lease())
-			require.Equal(t, roachpb.StoreID(2), rng.Lease().Replica.StoreID)
+			rng, err := ds.rangeCache.TestingGetCached(ctx, testUserRangeDescriptor.StartKey, false /* inverted */)
+			require.NoError(t, err)
+			require.NotNil(t, rng.Lease)
+			require.Equal(t, roachpb.StoreID(2), rng.Lease.Replica.StoreID)
 		})
 	}
 }
@@ -4250,10 +4242,11 @@ func TestEvictMetaRange(t *testing.T) {
 		}
 
 		// Verify that there is one meta2 cached range.
-		cachedRange := ds.rangeCache.GetCached(ctx, keys.RangeMetaKey(roachpb.RKey("a")), false)
-		if !cachedRange.Desc().StartKey.Equal(keys.Meta2Prefix) || !cachedRange.Desc().EndKey.Equal(testMetaEndKey) {
+		cachedRange, err := ds.rangeCache.TestingGetCached(ctx, keys.RangeMetaKey(roachpb.RKey("a")), false)
+		require.NoError(t, err)
+		if !cachedRange.Desc.StartKey.Equal(keys.Meta2Prefix) || !cachedRange.Desc.EndKey.Equal(testMetaEndKey) {
 			t.Fatalf("expected cached meta2 range to be [%s, %s), actual [%s, %s)",
-				keys.Meta2Prefix, testMetaEndKey, cachedRange.Desc().StartKey, cachedRange.Desc().EndKey)
+				keys.Meta2Prefix, testMetaEndKey, cachedRange.Desc.StartKey, cachedRange.Desc.EndKey)
 		}
 
 		// Simulate a split on the meta2 range and mark it as stale.
@@ -4265,15 +4258,17 @@ func TestEvictMetaRange(t *testing.T) {
 		}
 
 		// Verify that there are two meta2 cached ranges.
-		cachedRange = ds.rangeCache.GetCached(ctx, keys.RangeMetaKey(roachpb.RKey("a")), false)
-		if !cachedRange.Desc().StartKey.Equal(keys.Meta2Prefix) || !cachedRange.Desc().EndKey.Equal(splitKey) {
+		cachedRange, err = ds.rangeCache.TestingGetCached(ctx, keys.RangeMetaKey(roachpb.RKey("a")), false)
+		require.NoError(t, err)
+		if !cachedRange.Desc.StartKey.Equal(keys.Meta2Prefix) || !cachedRange.Desc.EndKey.Equal(splitKey) {
 			t.Fatalf("expected cached meta2 range to be [%s, %s), actual [%s, %s)",
-				keys.Meta2Prefix, splitKey, cachedRange.Desc().StartKey, cachedRange.Desc().EndKey)
+				keys.Meta2Prefix, splitKey, cachedRange.Desc.StartKey, cachedRange.Desc.EndKey)
 		}
-		cachedRange = ds.rangeCache.GetCached(ctx, keys.RangeMetaKey(roachpb.RKey("b")), false)
-		if !cachedRange.Desc().StartKey.Equal(splitKey) || !cachedRange.Desc().EndKey.Equal(testMetaEndKey) {
+		cachedRange, err = ds.rangeCache.TestingGetCached(ctx, keys.RangeMetaKey(roachpb.RKey("b")), false)
+		require.NoError(t, err)
+		if !cachedRange.Desc.StartKey.Equal(splitKey) || !cachedRange.Desc.EndKey.Equal(testMetaEndKey) {
 			t.Fatalf("expected cached meta2 range to be [%s, %s), actual [%s, %s)",
-				splitKey, testMetaEndKey, cachedRange.Desc().StartKey, cachedRange.Desc().EndKey)
+				splitKey, testMetaEndKey, cachedRange.Desc.StartKey, cachedRange.Desc.EndKey)
 		}
 	})
 }
@@ -5291,9 +5286,8 @@ func TestSendToReplicasSkipsStaleReplicas(t *testing.T) {
 						},
 					},
 				})
-				ent, err := rc.Lookup(ctx, roachpb.RKeyMin)
+				tok, err := rc.LookupWithEvictionToken(ctx, roachpb.RKeyMin, rangecache.EvictionToken{}, false)
 				require.NoError(t, err)
-				tok := rc.MakeEvictionToken(&ent)
 
 				numCalled := 0
 				transportFn := func(_ context.Context, ba *kvpb.BatchRequest) (*kvpb.BatchResponse, error) {
@@ -5349,16 +5343,16 @@ func TestSendToReplicasSkipsStaleReplicas(t *testing.T) {
 				_, err = ds.sendToReplicas(ctx, ba, tok, false /* withCommit */)
 				require.IsType(t, &sendError{}, err)
 				require.Regexp(t, "NotLeaseHolderError", err)
-				cached := rc.GetCached(ctx, tc.initialDesc.StartKey, false /* inverted */)
-				require.NotNil(t, cached)
-				require.Equal(t, tc.updatedDesc, *cached.Desc())
+				cached, err := rc.TestingGetCached(ctx, tc.initialDesc.StartKey, false /* inverted */)
+				require.NoError(t, err)
+				require.Equal(t, tc.updatedDesc, cached.Desc)
 				require.Equal(t, tc.expReplicasTried, numCalled)
 				if tc.expLeaseholder == 0 {
 					// Check that the leaseholder is cleared out.
-					require.Nil(t, cached.Leaseholder())
+					require.True(t, cached.Lease.Empty())
 				} else {
-					require.NotNil(t, cached.Leaseholder())
-					require.Equal(t, tc.expLeaseholder, cached.Leaseholder().ReplicaID)
+					require.NotNil(t, cached.Lease)
+					require.Equal(t, tc.expLeaseholder, cached.Lease.Replica.ReplicaID)
 				}
 			})
 		})