Merge #76452 #76563

76452: util/tracing: eliminate allocation for WithParent(<empty>) r=andreimatei a=andreimatei

This patch makes WithRemoteParent(SpanMeta{}) not allocate a useless
no-op span creation option. WithRemoteParent(SpanMeta{}) is superflous,
but the call can happen when the caller that doesn't know whether
there's a parent or not. I don't think the case is very common, but the
optimization is very natural.
This patch makes the no-op option be a represented as `nil`, as opposed
to a heap-allocated empty struct.

BenchmarkSetupSpanForIncomingRPC is enhanced to cover more code paths.
The delta in the "/no_parent" case captures the improvement here - one
allocation saved per not-traced RPC when tracing is generally enabled.

```
name                                  old time/op    new time/op    delta
SetupSpanForIncomingRPC/traceInfo-32     434ns ± 2%     445ns ± 1%   +2.68%  (p=0.008 n=5+5)
SetupSpanForIncomingRPC/grpcMeta-32     1.22µs ± 3%    1.23µs ± 2%     ~     (p=1.000 n=5+5)
SetupSpanForIncomingRPC/no_parent-32     439ns ± 2%     312ns ± 1%  -28.90%  (p=0.008 n=5+5)

name                                  old alloc/op   new alloc/op   delta
SetupSpanForIncomingRPC/traceInfo-32      144B ± 0%      144B ± 0%     ~     (all equal)
SetupSpanForIncomingRPC/grpcMeta-32       544B ± 0%      544B ± 0%     ~     (all equal)
SetupSpanForIncomingRPC/no_parent-32      144B ± 0%       48B ± 0%  -66.67%  (p=0.008 n=5+5)

name                                  old allocs/op  new allocs/op  delta
SetupSpanForIncomingRPC/traceInfo-32      2.00 ± 0%      2.00 ± 0%     ~     (all equal)
SetupSpanForIncomingRPC/grpcMeta-32       4.00 ± 0%      4.00 ± 0%     ~     (all equal)
SetupSpanForIncomingRPC/no_parent-32      2.00 ± 0%      1.00 ± 0%  -50.00%  (p=0.008 n=5+5)
```

Release note: None

76563: rowenc: fix splitting lookup rows into family spans in some cases r=yuzefovich a=yuzefovich

Previously, we could incorrectly calculate whether fetching a KV for
`FamilyID==0` is needed. The zeroth KV is always present, and we rely on
it to find NULL values in columns in other column families. Before this
patch we could "optimize" the behavior to not fetch the zeroth column
family with composite non-nullable columns when we need to lookup
nullable column families (families that only contain nullable columns);
as a result, the lookup could come back empty when those columns only
have NULLs.

Consider the following example:
```
CREATE TABLE t (
  pk1 DECIMAL NOT NULL, pk2 INT8 NOT NULL, c1 INT8, c2 INT8,
  PRIMARY KEY (pk1, pk2),
  UNIQUE (pk2),
  FAMILY fam_c1 (c1), FAMILY fam_c2 (pk1, pk2, c2)
);
INSERT INTO t (pk1, pk2, c1, c2) VALUES (1:::DECIMAL, 0:::INT8, 0:::INT8);
```
When the INSERT statement is evaluated, only a KV entry for `fam_c1` is
actually put into the KV layer (because the value part of `fam_c2`  - `c2`
column - is NULL).

Next, when evaluating a query `SELECT c2 FROM t WHERE pk2 = 0;`, we
first will scan the secondary unique index to fetch `/1/0` primary
key. Then, we'll do an index join against the primary key to fetch `c2`.
Before this patch, we would perform a Get of `/t/pk/1/0/1/1` (essentially
trying to read `c2` directly of `fam_c2` - `/1/1` suffix is the varlen
encoding of family ID 1); however, there is no such entry, so we would
mistakenly think that the row is absent and return no rows.

The problem was that we incorrectly determined `fam_c2` to be
non-nullable, so we assumed it to always have a KV entry if a row is
present. We made that determination based on the fact that `pk1` column
(which is non-nullable, indexed, and composite) must force the existence
of that KV entry because we're using the primary index encoding.

However, I think that reasoning is just bogus. Any column family should
be considered non-nullable IFF it has a non-nullable non-indexed column,
so this patch removes all the business about non-nullable, indexed,
composite columns. The zeroth column family is an exception that it is
always non-nullable.

Note that this patch makes us fetch more column families, so it should
not be a correctness regression although it could be a performance
regression if my reasoning is wrong.

With this change in place we now correctly perform a scan of
`/t/pk/1/{0-1}` span.

Fixes: #76289.

Release note (bug fix): Previously, CockroachDB could incorrectly not
return a row from a table with multiple column families when that row
contains a NULL value when a composite type (FLOAT, DECIMAL, COLLATED
STRING, or an arrays of such a type) is included in the PRIMARY KEY. For
the bug to occur composite column from the PRIMARY KEY must be included
in any column family other than the first one.

Co-authored-by: Andrei Matei <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>

pkg/server/BUILD.bazel

@@ -432,6 +432,7 @@ go_test(
         "@org_golang_google_grpc//:go_default_library",
         "@org_golang_google_grpc//codes",
         "@org_golang_google_grpc//credentials",
+        "@org_golang_google_grpc//metadata",
         "@org_golang_google_grpc//status",
         "@org_golang_x_crypto//bcrypt",
     ],

pkg/server/bench_test.go

@@ -18,24 +18,48 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing"
-	"github.com/cockroachdb/cockroach/pkg/util/tracing/tracingpb"
+	"google.golang.org/grpc/metadata"
 )
 
 func BenchmarkSetupSpanForIncomingRPC(b *testing.B) {
 	skip.UnderDeadlock(b, "span reuse triggers false-positives in the deadlock detector")
 	defer leaktest.AfterTest(b)()
-	ctx := context.Background()
-	ba := &roachpb.BatchRequest{Header: roachpb.Header{TraceInfo: tracingpb.TraceInfo{
-		TraceID:       1,
-		ParentSpanID:  2,
-		RecordingMode: tracingpb.TraceInfo_NONE,
-	}}}
-	b.ReportAllocs()
-	tr := tracing.NewTracerWithOpt(ctx,
-		tracing.WithTracingMode(tracing.TracingModeActiveSpansRegistry),
-		tracing.WithSpanReusePercent(100))
-	for i := 0; i < b.N; i++ {
-		_, sp := setupSpanForIncomingRPC(ctx, roachpb.SystemTenantID, ba, tr)
-		sp.finish(ctx, nil /* br */)
+
+	for _, tc := range []struct {
+		name      string
+		traceInfo bool
+		grpcMeta  bool
+	}{
+		{name: "traceInfo", traceInfo: true},
+		{name: "grpcMeta", grpcMeta: true},
+		{name: "no parent"},
+	} {
+		b.Run(tc.name, func(b *testing.B) {
+			b.ReportAllocs()
+
+			ctx := context.Background()
+			tr := tracing.NewTracerWithOpt(ctx,
+				tracing.WithTracingMode(tracing.TracingModeActiveSpansRegistry),
+				tracing.WithSpanReusePercent(100))
+			parentSpan := tr.StartSpan("parent")
+			defer parentSpan.Finish()
+
+			ba := &roachpb.BatchRequest{}
+			if tc.traceInfo {
+				ba.TraceInfo = parentSpan.Meta().ToProto()
+			} else if tc.grpcMeta {
+				traceCarrier := tracing.MapCarrier{
+					Map: make(map[string]string),
+				}
+				tr.InjectMetaInto(parentSpan.Meta(), traceCarrier)
+				ctx = metadata.NewIncomingContext(ctx, metadata.New(traceCarrier.Map))
+			}
+
+			b.ResetTimer()
+			for i := 0; i < b.N; i++ {
+				_, sp := setupSpanForIncomingRPC(ctx, roachpb.SystemTenantID, ba, tr)
+				sp.finish(ctx, nil /* br */)
+			}
+		})
 	}
 }

pkg/sql/logictest/testdata/logic_test/select_index

@@ -673,3 +673,33 @@ SELECT k FROM t47976 WHERE
   (a > 4 OR b <= 5.23 OR c IN (1, 2, 3)) AND
   (a = 12 OR b = 15.23 OR c = 14) AND
   (a > 58 OR b < 0 OR c >= 13)
+
+# Regression test for incorrectly splitting a lookup row (with NULL values not
+# in the lookup key) into family spans (#76289).
+statement ok
+CREATE TABLE t76289_1 (
+  pk1 DECIMAL NOT NULL, pk2 INT8 NOT NULL, c1 INT8, c2 INT8,
+  PRIMARY KEY (pk1, pk2),
+  UNIQUE (pk2),
+  FAMILY fam_c1 (c1), FAMILY fam_c2 (pk1, pk2, c2)
+);
+INSERT INTO t76289_1 (pk1, pk2, c1) VALUES (1:::DECIMAL, 0:::INT8, 0:::INT8);
+
+query I
+SELECT c2 FROM t76289_1 WHERE pk2 = 0;
+----
+NULL
+
+statement ok
+CREATE TABLE t76289_2 (
+  pk1 DECIMAL NOT NULL, pk2 INT8 NOT NULL, c1 INT8, c2 INT8,
+  PRIMARY KEY (pk1, pk2),
+  INDEX (pk2),
+  FAMILY fam_c1 (c1), FAMILY fam_c2 (pk1, pk2, c2)
+);
+INSERT INTO t76289_2 (pk1, pk2, c1) VALUES (1:::DECIMAL, 0:::INT8, 0:::INT8);
+
+query I
+SELECT c2 FROM t76289_2@t76289_2_pk2_idx WHERE pk2 = 0;
+----
+NULL

pkg/sql/rowenc/index_encoding.go

@@ -286,18 +286,16 @@ func NeededColumnFamilyIDs(
 			if nc.Contains(columnOrdinal) {
 				needed = true
 			}
-			if !columns[columnOrdinal].IsNullable() && (!indexedCols.Contains(columnOrdinal) ||
-				compositeCols.Contains(columnOrdinal) && !hasSecondaryEncoding) {
-				// The column is non-nullable and cannot be decoded from a different
-				// family, so this column family must have a KV entry for every row.
+			if !columns[columnOrdinal].IsNullable() && !indexedCols.Contains(columnOrdinal) {
+				// This column is non-nullable and is not indexed, thus, it must
+				// be stored in the value part of the KV entry. As a result,
+				// this column family is non-nullable too.
 				nullable = false
 			}
 		}
 		if needed {
 			neededFamilyIDs = append(neededFamilyIDs, family.ID)
-			if !nullable {
-				allFamiliesNullable = false
-			}
+			allFamiliesNullable = allFamiliesNullable && nullable
 		}
 		return nil
 	})

pkg/util/tracing/span_options.go

@@ -254,8 +254,8 @@ type remoteParent SpanMeta
 // WithRemoteParentFromTraceInfo will not allocate (whereas
 // WithRemoteParentFromSpanMeta allocates).
 func WithRemoteParentFromSpanMeta(parent SpanMeta) SpanOption {
-	if parent.sterile {
-		return remoteParent{}
+	if parent.Empty() || parent.sterile {
+		return nil
 	}
 	return (remoteParent)(parent)
 }