Merge #73383 #73387

73383: tracing: fix a crash r=andreimatei a=andreimatei

When a child span finishes, it tries to deregister itself from the
parent. We assert that the parent has a reference to this child. This
assertion fired because there is a case where the parent does not have a
reference to the child - when the parent had too many open children at
the time when the child was created, it will not register the child. In
effect, such a child is a root. This patch makes the child in question
aware of the fact that it is really a root.

Fixes #73351

Release note: None

73387: sql/tests: ignore an expected error for RSG r=yuzefovich a=yuzefovich

Addresses: #70663 (comment).

Release note: None

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

pkg/sql/tests/rsg_test.go

@@ -543,6 +543,7 @@ var ignoredErrorPatterns = []string{
 	// TODO(mjibson): fix these
 	"column .* must appear in the GROUP BY clause or be used in an aggregate function",
 	"aggregate functions are not allowed in ON",
+	"ordered-set aggregations must have a WITHIN GROUP clause containing one ORDER BY column",
 }
 
 var ignoredRegex = regexp.MustCompile(strings.Join(ignoredErrorPatterns, "|"))

pkg/util/tracing/crdbspan.go

@@ -587,22 +587,31 @@ func (s *crdbSpan) getRecordingNoChildrenLocked(
 	return rs
 }
 
-func (s *crdbSpan) addChild(child *crdbSpan, collectChildRec bool) {
-	s.mu.Lock()
-	defer s.mu.Unlock()
+// addChildLocked adds a child to the receiver.
+//
+// The receiver's lock must be held.
+//
+// The adding fails if the receiver is not recording  (non-recording spans don't
+// accumulate children) or if the reciver has reached the maximum number of
+// spans it can keep track off. Returns false in these cases.
+func (s *crdbSpan) addChildLocked(child *crdbSpan, collectChildRec bool) bool {
+	s.mu.AssertHeld()
 	if s.recordingType() == RecordingOff {
 		// We're not recording; there's nothing to do. The caller also checks the
 		// recording status, but we want to also check it here under the lock
 		// (double-checked locking).
-		return
+		return false
 	}
 
-	if len(s.mu.recording.openChildren) < maxChildrenPerSpan {
-		s.mu.recording.openChildren = append(
-			s.mu.recording.openChildren,
-			childRef{crdbSpan: child, collectRecording: collectChildRec},
-		)
+	if len(s.mu.recording.openChildren) >= maxChildrenPerSpan {
+		return false
 	}
+
+	s.mu.recording.openChildren = append(
+		s.mu.recording.openChildren,
+		childRef{crdbSpan: child, collectRecording: collectChildRec},
+	)
+	return true
 }
 
 // childFinished is called when a child is Finish()ed. Depending on the

pkg/util/tracing/span_test.go

@@ -350,14 +350,19 @@ func TestSpanMaxChildren(t *testing.T) {
 	tr := NewTracer()
 	sp := tr.StartSpan("root", WithRecording(RecordingStructured))
 	defer sp.Finish()
-	for i := 0; i < maxChildrenPerSpan+123; i++ {
-		tr.StartSpan(fmt.Sprintf("child %d", i), WithParent(sp))
+	numChildren := maxChildrenPerSpan + 123
+	children := make([]*Span, numChildren)
+	for i := 0; i < numChildren; i++ {
+		children[i] = tr.StartSpan(fmt.Sprintf("child %d", i), WithParent(sp))
 		exp := i + 1
 		if exp > maxChildrenPerSpan {
 			exp = maxChildrenPerSpan
 		}
 		require.Len(t, sp.i.crdb.mu.recording.openChildren, exp)
 	}
+	for _, s := range children {
+		s.Finish()
+	}
 }
 
 type explodyNetTr struct {

pkg/util/tracing/tracer.go

@@ -740,9 +740,16 @@ func (t *Tracer) startSpanGeneric(
 		// created Span to the parent so that the parent will later be able to
 		// collect the child's recording.
 		if opts.Parent != nil && opts.Parent.i.crdb != nil {
-			if opts.Parent.i.crdb.recordingType() != RecordingOff {
-				s.i.crdb.mu.parent = opts.Parent.i.crdb
-				opts.Parent.i.crdb.addChild(s.i.crdb, !opts.ParentDoesNotCollectRecording)
+			parent := opts.Parent.i.crdb
+			if parent.recordingType() != RecordingOff {
+				// We're going to hold the parent's lock while we link both the parent
+				// to the child and the child to the parent.
+				parent.withLock(func() {
+					added := parent.addChildLocked(s.i.crdb, !opts.ParentDoesNotCollectRecording)
+					if added {
+						s.i.crdb.mu.parent = opts.Parent.i.crdb
+					}
+				})
 			}
 		}
 		s.i.crdb.enableRecording(opts.recordingType())