Merge #150115

150115: sql: harden stmt bundle building code a bit r=yuzefovich a=yuzefovich

We just saw a sentry report where on explicit stmt bundle collection via EXPLAIN ANALYZE (DEBUG) we hit a nil pointer because `flowInfo.diagram` was nil. I've audited the code and don't know how this could've happened, so this commit hardens the code a bit by adding nil checks (and crashing in the test-only builds).

Fixes: #149987.

Release note: None

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

Author: craig[bot]

pkg/sql/explain_bundle.go

@@ -434,6 +434,12 @@ func (b *stmtBundleBuilder) addDistSQLDiagrams() {
 	}
 
 	for i, d := range b.plan.distSQLFlowInfos {
+		if d.diagram == nil {
+			if buildutil.CrdbTestBuild {
+				panic(errors.AssertionFailedf("diagram shouldn't be nil when building the bundle"))
+			}
+			continue
+		}
 		d.diagram.AddSpans(b.trace)
 		_, url, err := d.diagram.ToURL()
 

pkg/sql/instrumentation.go

@@ -942,12 +942,19 @@ func (ih *instrumentationHelper) setExplainAnalyzeResult(
 			} else {
 				buf.WriteString("Diagram: ")
 			}
-			d.diagram.AddSpans(trace)
-			_, url, err := d.diagram.ToURL()
-			if err != nil {
-				buf.WriteString(err.Error())
+			if d.diagram != nil {
+				d.diagram.AddSpans(trace)
+				_, url, err := d.diagram.ToURL()
+				if err != nil {
+					buf.WriteString(err.Error())
+				} else {
+					buf.WriteString(url.String())
+				}
 			} else {
-				buf.WriteString(url.String())
+				if buildutil.CrdbTestBuild {
+					panic(errors.AssertionFailedf("diagram shouldn't be nil in EXPLAIN ANALYZE (DISTSQL)"))
+				}
+				buf.WriteString("<missing>")
 			}
 			rows = append(rows, buf.String())
 		}

pkg/sql/plan.go

@@ -320,7 +320,10 @@ type planNodeSpooled interface {
 var _ planNodeSpooled = &spoolNode{}
 
 type flowInfo struct {
-	typ     planComponentType
+	typ planComponentType
+	// diagram is only populated when instrumentationHelper.shouldSaveDiagrams()
+	// returns true. (Even in that case we've seen a sentry report #149987 where
+	// it was nil.)
 	diagram execinfrapb.FlowDiagram
 	// explainVec and explainVecVerbose are only populated when collecting a
 	// statement bundle when the plan was vectorized.