Merge #29077 #29103 #29121

29077: backport-2.1: storage,kv: remove some allocations r=jordanlewis a=jordanlewis

Backport 1/1 commits from #29075.

/cc @cockroachdb/release

---

Guard an expensive log.V in a log.ExpensiveLogEnabled.

Pass roachpb.Lease by value in one situation in which allocating it is
pointless.

cc @benesch

This seems to improve read-only kv performance by almost 5%!

Release note: None


29103: release-2.1: base: make TestValidateAddrs portable r=knz a=knz

Backport 1/1 commits from #29101.

/cc @cockroachdb/release

---

On (at least) osx the port name resolution error is different. This patch
modifies the test to catch that.

Release note: None


29121: release-2.1: opt: fix LookupJoinDef interning, add tests r=RaduBerinde a=RaduBerinde

Backport 1/1 commits from #29117.

/cc @cockroachdb/release

---

Fixing an omission I noticed in internLookupJoinDef and adding missing
tests for interning defs.

Release note: None


Co-authored-by: Jordan Lewis <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>

Author: 4 people

pkg/base/addr_validation_test.go

@@ -60,6 +60,21 @@ func TestValidateAddrs(t *testing.T) {
 		localAddr = "[" + localAddr + "]"
 	}
 
+	// We need to know what a port name resolution error look like,
+	// because they may be different across systems/libcs.
+	_, err = net.DefaultResolver.LookupPort(context.Background(), "tcp", "nonexistent")
+	if err == nil {
+		t.Fatal("expected port resolution failure, got no error")
+	}
+	portExpectedErr := err.Error()
+	// For the host name resolution error we can reliably expect "no such host"
+	// below, but before we test anything we need to ensure we indeed have
+	// a reliably non-resolvable host name.
+	_, err = net.DefaultResolver.LookupIPAddr(context.Background(), "nonexistent.example.com")
+	if err == nil {
+		t.Fatal("expected host resolution failure, got no error")
+	}
+
 	// The test cases.
 	testData := []struct {
 		in          addrs
@@ -120,7 +135,7 @@ func TestValidateAddrs(t *testing.T) {
 		{addrs{":26257", "", "localhost", ""}, "invalid --http-addr.*missing port in address", addrs{}},
 		// Invalid port number.
 		{addrs{"localhost:-1231", "", "", ""}, "invalid port", addrs{}},
-		{addrs{"localhost:nonexistent", "", "", ""}, "unknown port", addrs{}},
+		{addrs{"localhost:nonexistent", "", "", ""}, portExpectedErr, addrs{}},
 		// Invalid address.
 		{addrs{"nonexistent.example.com:26257", "", "", ""}, "no such host", addrs{}},
 		{addrs{"333.333.333.333:26257", "", "", ""}, "no such host", addrs{}},

pkg/kv/dist_sender.go

@@ -1279,7 +1279,9 @@ func (ds *DistSender) sendToReplicas(
 	}
 
 	curReplica := transport.NextReplica()
-	log.VEventf(ctx, 2, "r%d: sending batch %s to %s", rangeID, args.Summary(), curReplica)
+	if log.ExpensiveLogEnabled(ctx, 2) {
+		log.VEventf(ctx, 2, "r%d: sending batch %s to %s", rangeID, args.Summary(), curReplica)
+	}
 	br, err := transport.SendNext(ctx)
 
 	// This loop will retry operations that fail with errors that reflect

pkg/sql/opt/memo/private_storage.go

@@ -315,7 +315,9 @@ func (ps *privateStorage) internLookupJoinDef(def *LookupJoinDef) PrivateID {
 	// The below code is carefully constructed to not allocate in the case where
 	// the value is already in the map. Be careful when modifying.
 	ps.keyBuf.Reset()
+	ps.keyBuf.writeUvarint(uint64(def.JoinType))
 	ps.keyBuf.writeUvarint(uint64(def.Table))
+	ps.keyBuf.writeUvarint(uint64(def.Index))
 	ps.keyBuf.writeColList(def.KeyCols)
 	// Add a separator between the list and the set. Note that the column IDs
 	// cannot be 0.