Merge #116329 #117570

116329: go.mod: update comments for golang.org/x r=dt a=dt

This is a comment-only change that highlights that golang.org/x dependencies are treated like part of go w.r.t. change review.

Release note: none.
Epic: none.

117570: util/span: improve span frontier out of bounds check r=miretskiy a=jayshrivastava

Previously, when the span frontier running in strict mode was forwarded using a span
which is out of bounds, the error did not include the frontier itself, making it hard
to debug such errors. Now the error message includes the frontier.

Also, this change updates the `String()` method on the llrb frontier, making it consistent
with the btree frontier. It also changes the error message in the llrb frontier to be
consistent with the b tree frontier error message.

Epic: None
Informs: #117476
Release note: None

Co-authored-by: David Taylor <[email protected]>
Co-authored-by: Jayant Shrivastava <[email protected]>

go.mod

@@ -2,6 +2,27 @@ module github.com/cockroachdb/cockroach
 
 go 1.21
 
+// golang.org/x/* packages are maintained and curated by the go project, just
+// without the backwards compatibility promises the standard library, and thus
+// should be treated like part of the go standard library. Accordingly upgrades
+// to golang.org/x packages are treated similarly to go upgrades: they’re
+// considered sweeping changes, are avoided in backports, and following the
+// merge of any upgrades we should communicate to all teams to be on the lookout
+// for behavior changes, just like we would after a go upgrade.
+require (
+	golang.org/x/crypto v0.16.0
+	golang.org/x/exp v0.0.0-20230626212559-97b1e661b5df
+	golang.org/x/exp/typeparams v0.0.0-20221208152030-732eee02a75a // indirect
+	golang.org/x/mod v0.14.0 // indirect
+	golang.org/x/net v0.19.0
+	golang.org/x/oauth2 v0.5.0
+	golang.org/x/sync v0.5.0
+	golang.org/x/sys v0.15.0
+	golang.org/x/text v0.14.0
+	golang.org/x/time v0.3.0
+	golang.org/x/tools v0.16.0
+)
+
 // The following dependencies are key infrastructure dependencies and
 // should be updated as their own commit (i.e. not bundled with a dep
 // upgrade to something else), and reviewed by a broader community of
@@ -16,17 +37,6 @@ require (
 	github.com/google/btree v1.0.1
 	github.com/google/pprof v0.0.0-20210827144239-02619b876842
 	github.com/google/uuid v1.3.0
-	golang.org/x/crypto v0.16.0
-	golang.org/x/exp v0.0.0-20230626212559-97b1e661b5df
-	golang.org/x/exp/typeparams v0.0.0-20221208152030-732eee02a75a // indirect
-	golang.org/x/mod v0.14.0 // indirect
-	golang.org/x/net v0.19.0
-	golang.org/x/oauth2 v0.5.0
-	golang.org/x/sync v0.5.0
-	golang.org/x/sys v0.15.0
-	golang.org/x/text v0.14.0
-	golang.org/x/time v0.3.0
-	golang.org/x/tools v0.16.0
 	google.golang.org/api v0.110.0
 	google.golang.org/genproto v0.0.0-20230227214838-9b19f0bdc514
 	google.golang.org/grpc v1.53.0

pkg/util/span/frontier.go

@@ -116,6 +116,14 @@ var useBtreeFrontier = envutil.EnvOrDefaultBool("COCKROACH_BTREE_SPAN_FRONTIER_E
 // span forwarding even if the span is not a subset of this frontier.
 var spanMustBeTracked = envutil.EnvOrDefaultBool("COCKROACH_SPAN_FRONTIER_STRICT_MODE_ENABLED", false)
 
+func enableStrictMode(enabled bool) func() {
+	old := spanMustBeTracked
+	spanMustBeTracked = enabled
+	return func() {
+		spanMustBeTracked = old
+	}
+}
+
 func enableBtreeFrontier(enabled bool) func() {
 	old := useBtreeFrontier
 	useBtreeFrontier = enabled
@@ -478,7 +486,8 @@ func (f *btreeFrontier) forward(span roachpb.Span, insertTS hlc.Timestamp) error
 		it.FirstOverlap(todoEntry)
 		if !it.Valid() {
 			if spanMustBeTracked {
-				return errors.Newf("span %s is not a sub-span of this frontier (remaining %s)", span, todoEntry.span())
+				return errors.Newf("span %s is not a sub-span of this frontier (remaining %s) (frontier %s)",
+					span, todoEntry.span(), f.String())
 			}
 			break
 		}
@@ -490,7 +499,8 @@ func (f *btreeFrontier) forward(span roachpb.Span, insertTS hlc.Timestamp) error
 		// This establishes the invariant that overlap start must be at or before todoEntry start.
 		if todoEntry.Start.Compare(overlap.Start) < 0 {
 			if spanMustBeTracked {
-				return errors.Newf("span %s is not a sub-span of this frontier (remaining %s)", span, todoEntry.span())
+				return errors.Newf("span %s is not a sub-span of this frontier (remaining %s) (frontier %s)",
+					span, todoEntry.span(), f.String())
 			}
 			todoEntry.Start = overlap.Start
 			if todoEntry.isEmptyRange() {

pkg/util/span/frontier_test.go

@@ -75,6 +75,59 @@ func makeFrontierForwarded(
 		}
 	}
 }
+
+func TestSpanFrontierStrictModeBTree(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer enableStrictMode(true)()
+
+	keyA, keyB, keyC := roachpb.Key("a"), roachpb.Key("b"), roachpb.Key("c")
+	keyD, keyE, keyF := roachpb.Key("d"), roachpb.Key("e"), roachpb.Key("f")
+	ts0 := hlc.Timestamp{}
+
+	testutils.RunTrueAndFalse(t, "btree", func(t *testing.T, useBtreeFrontier bool) {
+		defer enableBtreeFrontier(useBtreeFrontier)()
+		for i, tc := range []struct {
+			frontier  []roachpb.Span
+			forward   roachpb.Span
+			error     string
+			remaining string
+		}{
+			{
+				frontier: []roachpb.Span{{Key: keyB, EndKey: keyD}},
+				forward:  roachpb.Span{Key: keyC, EndKey: keyE},
+				error:    "span {c-e} is not a sub-span of this frontier (remaining {d-e}) (frontier [{b-d}@0,0])",
+			},
+			{
+				frontier: []roachpb.Span{{Key: keyB, EndKey: keyD}},
+				forward:  roachpb.Span{Key: keyE, EndKey: keyF},
+				error:    "span {e-f} is not a sub-span of this frontier (remaining {e-f}) (frontier [{b-d}@0,0])",
+			},
+			{
+				frontier: []roachpb.Span{{Key: keyB, EndKey: keyD}},
+				forward:  roachpb.Span{Key: keyA, EndKey: keyB},
+				error:    "span {a-b} is not a sub-span of this frontier (remaining {a-b}) (frontier [{b-d}@0,0])",
+			},
+			{
+				frontier: []roachpb.Span{{Key: keyB, EndKey: keyD}},
+				forward:  roachpb.Span{Key: keyA, EndKey: keyE},
+				error:    "span {a-e} is not a sub-span of this frontier (remaining {a-e}) (frontier [{b-d}@0,0])",
+			},
+			{
+				frontier: []roachpb.Span{{Key: keyA, EndKey: keyB}, {Key: keyC, EndKey: keyD}},
+				forward:  roachpb.Span{Key: keyA, EndKey: keyD},
+				error:    "span {a-d} is not a sub-span of this frontier (remaining {b-d}) (frontier [{a-b}@0,0] [{c-d}@0,0])",
+			},
+		} {
+			t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
+				f, err := MakeFrontier(tc.frontier...)
+				require.NoError(t, err)
+				_, err = f.Forward(tc.forward, ts0)
+
+				require.EqualError(t, err, tc.error)
+			})
+		}
+	})
+}
 func TestSpanFrontier(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 

pkg/util/span/llrb_frontier.go

@@ -48,7 +48,7 @@ func (s *llrbFrontierEntry) Range() interval.Range {
 }
 
 func (s *llrbFrontierEntry) String() string {
-	return fmt.Sprintf("[%s @ %s]", s.span, s.ts)
+	return fmt.Sprintf("[%s@%s]", s.span, s.ts)
 }
 
 // llrbFrontierHeap implements heap.Interface and holds `llrbFrontierEntry`s. Entries
@@ -358,7 +358,8 @@ func (f *llrbFrontier) insert(span roachpb.Span, insertTS hlc.Timestamp) error {
 
 	if spanMustBeTracked {
 		if todoRange.Start.Compare(todoRange.End) < 0 {
-			return errors.Newf("span %s is not a sub-span of this frontier (remaining %s)", span, todoRange)
+			return errors.Newf("span %s is not a sub-span of this frontier (remaining {%s-%s}) (frontier %s)",
+				span, todoRange.Start, todoRange.End, f.String())
 		}
 	}