Merge #72965 #73068

72965: catalog/tabledesc: calculate KeysPerRow precisely for secondary indexes r=yuzefovich a=yuzefovich

Previously, we would say that a secondary index with a STORING clause
might have the same number of KVs per row as there are column families
in the table. However, this is not as precise as possible - namely, the
secondary index always uses the 0th column family, but 1st, 2nd, etc
column family is only used if at least one column from the family is in
the STORING clause (if not, that column family doesn't get a KV in the
secondary index). This observation allows us to compute `KeysPerRow`
precisely which then allows us to set smaller limits on the
BatchRequests in the fetchers.

Fixes: #72739

Release note: None

73068: kv,sql: fix some nilness lint errors r=tbg a=rickystewart

The `nilness` linter caught these cases. We remove some unnecessary
nilness checks.

Release note: None

Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>

pkg/kv/kvserver/replica_proposal_buf.go

@@ -521,9 +521,7 @@ func (b *propBuf) FlushLockedWithRaftGroup(
 		// only after performing necessary bookkeeping.
 		if filter := b.testing.submitProposalFilter; filter != nil {
 			if drop, err := filter(p); drop || err != nil {
-				if firstErr == nil {
-					firstErr = err
-				}
+				firstErr = err
 				continue
 			}
 		}

pkg/sql/catalog/tabledesc/structured.go

@@ -134,10 +134,25 @@ func (desc *wrapper) KeysPerRow(indexID descpb.IndexID) (int, error) {
 	if err != nil {
 		return 0, err
 	}
-	if idx.NumSecondaryStoredColumns() == 0 {
+	if idx.NumSecondaryStoredColumns() == 0 || len(desc.Families) == 1 {
 		return 1, nil
 	}
-	return len(desc.Families), nil
+	// Calculate the number of column families used by the secondary index. We
+	// only need to look at the stored columns because column families are only
+	// applicable to the value part of the KV.
+	//
+	// 0th family is always present.
+	numUsedFamilies := 1
+	storedColumnIDs := idx.CollectSecondaryStoredColumnIDs()
+	for _, family := range desc.Families[1:] {
+		for _, columnID := range family.ColumnIDs {
+			if storedColumnIDs.Contains(columnID) {
+				numUsedFamilies++
+				break
+			}
+		}
+	}
+	return numUsedFamilies, nil
 }
 
 // BuildIndexName returns an index name that is not equal to any

pkg/sql/catalog/tabledesc/structured_test.go

@@ -654,11 +654,49 @@ func TestKeysPerRow(t *testing.T) {
 		indexID     descpb.IndexID
 		expected    int
 	}{
-		{"(a INT PRIMARY KEY, b INT, INDEX (b))", 1, 1},                                     // Primary index
-		{"(a INT PRIMARY KEY, b INT, INDEX (b))", 2, 1},                                     // 'b' index
-		{"(a INT PRIMARY KEY, b INT, FAMILY (a), FAMILY (b), INDEX (b))", 1, 2},             // Primary index
-		{"(a INT PRIMARY KEY, b INT, FAMILY (a), FAMILY (b), INDEX (b))", 2, 1},             // 'b' index
-		{"(a INT PRIMARY KEY, b INT, FAMILY (a), FAMILY (b), INDEX (a) STORING (b))", 2, 2}, // 'a' index
+		{
+			"(a INT PRIMARY KEY, b INT, INDEX (b))",
+			1, // Primary index
+			1,
+		},
+		{
+			"(a INT PRIMARY KEY, b INT, INDEX (b))",
+			2, // 'b' index
+			1,
+		},
+		{
+			"(a INT PRIMARY KEY, b INT, FAMILY (a), FAMILY (b), INDEX (b))",
+			1, // Primary index
+			2,
+		},
+		{
+			"(a INT PRIMARY KEY, b INT, FAMILY (a), FAMILY (b), INDEX (b))",
+			2, // 'b' index
+			1,
+		},
+		{
+			"(a INT PRIMARY KEY, b INT, FAMILY (a), FAMILY (b), INDEX (a) STORING (b))",
+			2, // 'a' index
+			2,
+		},
+		{
+			"(a INT, b INT, c INT, d INT, e INT, FAMILY f0 (a, b), " +
+				"FAMILY f1 (c), FAMILY f2(d, e), INDEX (d) STORING (b))",
+			2, // 'd' index
+			1, // Only f0 is needed.
+		},
+		{
+			"(a INT, b INT, c INT, d INT, e INT, FAMILY f0 (a, b), " +
+				"FAMILY f1 (c), FAMILY f2(d, e), INDEX (d) STORING (c, e))",
+			2, // 'd' index
+			3, // f1 and f2 are needed, but f0 is always present.
+		},
+		{
+			"(a INT, b INT, c INT, d INT, e INT, FAMILY f0 (a, b), " +
+				"FAMILY f1 (c), FAMILY f2(d, e), INDEX (a, c) STORING (d))",
+			2, // 'a, c' index
+			2, // Only f2 is needed, but f0 is always present.
+		},
 	}
 
 	for i, test := range tests {

pkg/sql/pgwire/auth.go

@@ -172,11 +172,6 @@ func (c *conn) handleAuthentication(
 		))
 	}
 
-	if err != nil {
-		ac.LogAuthFailed(ctx, eventpb.AuthFailReason_METHOD_NOT_FOUND, err)
-		return connClose, sendError(pgerror.WithCandidateCode(err, pgcode.InvalidAuthorizationSpecification))
-	}
-
 	// Set up lazy provider for password or cert-password methods.
 	pwDataFn := func(ctx context.Context) ([]byte, *tree.DTimestamp, error) {
 		pwHash, err := pwRetrievalFn(ctx)

pkg/sql/rowflow/input_sync.go

@@ -233,7 +233,7 @@ func (s *serialOrderedSynchronizer) consumeMetadata(
 			src.row = row
 			return nil
 		}
-		if row == nil && meta == nil {
+		if row == nil {
 			return nil
 		}
 	}

pkg/util/timeutil/time_zone_util.go

@@ -138,10 +138,6 @@ func ParseTimeZoneOffset(
 		offset = hoursMinutesSecondsToSeconds(origRepr)
 		offset *= offsetMultiplier
 
-		if err != nil {
-			return 0, "", false
-		}
-
 		return offset, location, true
 	}