From 11898446e8016c2a9c91f95b0d7a0c18013acf61 Mon Sep 17 00:00:00 2001
From: Yahor Yuzefovich <yahor@cockroachlabs.com>
Date: Tue, 30 Apr 2024 18:06:55 -0700
Subject: [PATCH] sql: improve ExplainRedact tests

This commit improves the EXPLAIN (REDACT) tests in the following manner:
- only log successful statements with semicolon in the end
- ensure that all generated statements are sound (i.e. don't error on
`EXPLAIN stmt`)
- use simple names (we can't inject the PII into the names anyway)
- include the stmt for non-internal errors.

Release note: None
---
 pkg/ccl/testccl/sqlccl/explain_test.go |  7 +++----
 pkg/sql/explain_test.go                |  1 +
 pkg/sql/tests/explain_test_util.go     | 20 +++++++++++++++-----
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/pkg/ccl/testccl/sqlccl/explain_test.go b/pkg/ccl/testccl/sqlccl/explain_test.go
index e1815240a432..78b3cef42f9b 100644
--- a/pkg/ccl/testccl/sqlccl/explain_test.go
+++ b/pkg/ccl/testccl/sqlccl/explain_test.go
@@ -44,7 +44,6 @@ func TestExplainRedactDDL(t *testing.T) {
 
 	s, sqlDB, _ := serverutils.StartServer(t, base.TestServerArgs{})
 	defer s.Stopper().Stop(ctx)
-	defer sqlDB.Close()
 
 	query := func(sql string) (*gosql.Rows, error) {
 		return sqlDB.QueryContext(ctx, sql)
@@ -69,12 +68,12 @@ func TestExplainRedactDDL(t *testing.T) {
 	setup = append(setup, "SET CLUSTER SETTING sql.stats.automatic_collection.enabled = off;")
 	setup = append(setup, "SET statement_timeout = '5s';")
 	for _, stmt := range setup {
-		t.Log(stmt)
 		if _, err := sqlDB.ExecContext(ctx, stmt); err != nil {
 			// Ignore errors.
-			t.Log("-- ignoring error:", err)
 			continue
 		}
+		// Only log successful statements.
+		t.Log(stmt + ";")
 	}
 
 	// Check EXPLAIN (OPT, CATALOG, REDACT) for each table.
@@ -92,7 +91,6 @@ func TestExplainRedactDDL(t *testing.T) {
 	}
 	for _, table := range tables {
 		explain := "EXPLAIN (OPT, CATALOG, REDACT) SELECT * FROM " + lexbase.EscapeSQLIdent(table)
-		t.Log(explain)
 		rows, err = query(explain)
 		if err != nil {
 			// This explain should always succeed.
@@ -119,6 +117,7 @@ func TestExplainRedactDDL(t *testing.T) {
 		sqlsmith.PrefixStringConsts(pii),
 		sqlsmith.OnlySingleDMLs(),
 		sqlsmith.EnableAlters(),
+		sqlsmith.SimpleNames(),
 	)
 	if err != nil {
 		t.Fatal(err)
diff --git a/pkg/sql/explain_test.go b/pkg/sql/explain_test.go
index 1e54caa6d514..11823313990e 100644
--- a/pkg/sql/explain_test.go
+++ b/pkg/sql/explain_test.go
@@ -551,6 +551,7 @@ func TestExplainRedact(t *testing.T) {
 	smith, err := sqlsmith.NewSmither(sqlDB, rng,
 		sqlsmith.PrefixStringConsts(pii),
 		sqlsmith.DisableDDLs(),
+		sqlsmith.SimpleNames(),
 	)
 	if err != nil {
 		t.Fatal(err)
diff --git a/pkg/sql/tests/explain_test_util.go b/pkg/sql/tests/explain_test_util.go
index 17aa5ca036a4..95c9a8e3ba7c 100644
--- a/pkg/sql/tests/explain_test_util.go
+++ b/pkg/sql/tests/explain_test_util.go
@@ -32,11 +32,21 @@ func GenerateAndCheckRedactedExplainsForPII(
 	containsPII func(explain, output string) error,
 ) {
 	// Generate a few random statements.
-	statements := make([]string, num)
+	statements := make([]string, 0, num)
 	t.Log("generated statements:")
-	for i := range statements {
-		statements[i] = smith.Generate()
-		t.Log(statements[i])
+	for len(statements) < num {
+		stmt := smith.Generate()
+		// Try vanilla EXPLAIN on this stmt to ensure that it is sound.
+		_, err := query("EXPLAIN " + stmt)
+		if err != nil {
+			// We shouldn't see any internal errors - ignore all others.
+			if strings.Contains(err.Error(), "internal error") {
+				t.Error(err)
+			}
+		} else {
+			statements = append(statements, stmt)
+			t.Log(stmt + ";")
+		}
 	}
 
 	// Gather EXPLAIN variants to test.
@@ -98,7 +108,7 @@ func GenerateAndCheckRedactedExplainsForPII(
 									} else if !strings.Contains(msg, "syntax error") {
 										// Skip logging syntax errors, since they're expected to be
 										// common and uninteresting.
-										t.Logf("encountered non-internal error: %s\n", err)
+										t.Logf("encountered non-internal error: %s\n%s\n\n", err, explain)
 									}
 									continue
 								}