From 9a2494498777bb701dc871ab5a50d001e8202e58 Mon Sep 17 00:00:00 2001
From: Marcus Gartner <marcus@cockroachlabs.com>
Date: Tue, 22 Jun 2021 15:22:16 -0700
Subject: [PATCH] sqlsmith: fix flaky TestGenerateParse

This commit fixes the `sqlsmith.TestGenerateParse` test that has been
flaking since #66599 when primary index stored columns were added to the
output of `SHOW INDEXES`.

sqlsmith uses `SHOW INDEXES` to build an in-memory representation of
indexes to help generate interesting randomized queries. It is not
equipped to handle `rowid` primary indexes, so rows output from
`SHOW INDEXES` with a `column_name` of `rowid` are filtered out. Stored
columns of primary indexes, now included in `SHOW INDEXES`, are not
filtered out, causing sqlsmith to build an in-memory representation of
`rowid` indexes that have only stored columns and no indexed columns.
This breaks the assumption within sqlsmith that each index should have
at least one indexed column, causing panics.

This commit fixes the issue by removing indexes with no indexed columns
from sqlsmith's in-memory index map.

Fixes #66723

Release note: None
---
 pkg/internal/sqlsmith/BUILD.bazel      | 1 -
 pkg/internal/sqlsmith/schema.go        | 8 ++++++++
 pkg/internal/sqlsmith/sqlsmith_test.go | 2 --
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/pkg/internal/sqlsmith/BUILD.bazel b/pkg/internal/sqlsmith/BUILD.bazel
index 9eee5196ce69..46251e262c90 100644
--- a/pkg/internal/sqlsmith/BUILD.bazel
+++ b/pkg/internal/sqlsmith/BUILD.bazel
@@ -52,7 +52,6 @@ go_test(
         "//pkg/server",
         "//pkg/sql/parser",
         "//pkg/testutils/serverutils",
-        "//pkg/testutils/skip",
         "//pkg/testutils/sqlutils",
         "//pkg/testutils/testcluster",
         "//pkg/util/leaktest",
diff --git a/pkg/internal/sqlsmith/schema.go b/pkg/internal/sqlsmith/schema.go
index 46880a5c261e..1655545aa2ae 100644
--- a/pkg/internal/sqlsmith/schema.go
+++ b/pkg/internal/sqlsmith/schema.go
@@ -438,6 +438,14 @@ func (s *Smither) extractIndexes(
 		if err := rows.Err(); err != nil {
 			return nil, err
 		}
+		// Remove indexes with empty Columns. This is the case for rowid indexes
+		// where the only index column, rowid, is ignored in the SQL statement
+		// above, but the stored columns are not.
+		for name, idx := range indexes {
+			if len(idx.Columns) == 0 {
+				delete(indexes, name)
+			}
+		}
 		ret[*t.TableName] = indexes
 	}
 	return ret, nil
diff --git a/pkg/internal/sqlsmith/sqlsmith_test.go b/pkg/internal/sqlsmith/sqlsmith_test.go
index 59645b253f8e..a1ba39800249 100644
--- a/pkg/internal/sqlsmith/sqlsmith_test.go
+++ b/pkg/internal/sqlsmith/sqlsmith_test.go
@@ -21,7 +21,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/ccl/utilccl"
 	"github.com/cockroachdb/cockroach/pkg/sql/parser"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
-	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/randutil"
@@ -140,7 +139,6 @@ func TestRandTableInserts(t *testing.T) {
 // sometimes put them into bad states that the parser would never do.
 func TestGenerateParse(t *testing.T) {
 	defer leaktest.AfterTest(t)()
-	skip.WithIssue(t, 66723, "flaky test")
 	defer utilccl.TestingEnableEnterprise()()
 
 	ctx := context.Background()