Merge #61807 #61857

61807: sqlsmith: fix error handling in makeAlter and panic handling r=fqazi a=fqazi

Fixes: #61775

Previously, the error from ReloadSchemas was ignored
and the statement generator would blindly continue.
This lead to sqlsmith failing completely and panicking
inside roach test. To address this, this patch fixes
the error handling in this case and makes the sqlsmith
error handling more robust to panics.

Release note: None

61857: bench/ddl_analysis: skip because it's flakey r=ajwerner a=ajwerner

I stressed this thing a lot. I guess not enough.

Touch #61856. 

Release note: None

Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com>
Co-authored-by: Andrew Werner <awerner32@gmail.com>

pkg/bench/ddl_analysis/validate_benchmark_data_test.go

@@ -68,6 +68,7 @@ func TestBenchmarkExpectation(t *testing.T) {
 	skip.UnderRace(t)
 	skip.UnderShort(t)
 	skip.UnderMetamorphic(t)
+	skip.WithIssue(t, 61856)
 
 	expecations := readExpectationsFile(t)
 

pkg/cmd/roachtest/sqlsmith.go

@@ -21,6 +21,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/internal/sqlsmith"
 	"github.com/cockroachdb/cockroach/pkg/util/randutil"
+	"github.com/cockroachdb/errors"
 )
 
 func registerSQLSmith(r *testRegistry) {
@@ -159,10 +160,29 @@ func registerSQLSmith(r *testRegistry) {
 				return
 			default:
 			}
-			stmt := smither.Generate()
+
+			stmt := ""
 			err := func() error {
 				done := make(chan error, 1)
 				go func(context.Context) {
+					defer func() {
+						if r := recover(); r != nil {
+							done <- errors.Newf("Caught error %s", r)
+							return
+						}
+					}()
+
+					// Generate can potentially panic in bad cases, so
+					// to avoid Go routines from dying we are going
+					// catch that here, and only pass the error into
+					// the channel.
+					stmt = smither.Generate()
+					if stmt == "" {
+						// If an empty statement is generated, then ignore it.
+						done <- errors.Newf("Empty statement returned by generate")
+						return
+					}
+
 					// At the moment, CockroachDB doesn't support pgwire query
 					// cancellation which is needed for correct handling of context
 					// cancellation, so instead of using a context with timeout, we opt
@@ -201,6 +221,11 @@ func registerSQLSmith(r *testRegistry) {
 					// a non-gateway node has crashed.
 					logStmt(stmt)
 					t.Fatalf("error: %s\nstmt:\n%s;", err, stmt)
+				} else if strings.Contains(es, "Empty statement returned by generate") ||
+					stmt == "" {
+					// Either were unable to generate a statement or
+					// we panicked making one.
+					t.Fatalf("Failed generating a query %s", err)
 				}
 				// Ignore other errors because they happen so
 				// frequently (due to sqlsmith not crafting

pkg/internal/sqlsmith/alter.go

@@ -52,7 +52,14 @@ func makeAlter(s *Smither) (tree.Statement, bool) {
 		// up the change). This has the added benefit of leaving
 		// behind old column references for a bit, which should
 		// test some additional logic.
-		_ = s.ReloadSchemas()
+		err := s.ReloadSchemas()
+		if err != nil {
+			// If we fail to load any schema information, then
+			// the actual statement generation could panic, so
+			// fail out here.
+			return nil, false
+		}
+
 		for i := 0; i < retryCount; i++ {
 			stmt, ok := s.alterSampler.Next()(s)
 			if ok {