Merge #130895 #130896

130895: tree: fix walkStmt for SHOW EXPERIMENTAL_FINGERPRINTS r=rafiss a=rafiss If SHOW EXPERIMENTAL_FINGERPRINTS FROM TABLE appeared in certain kinds of subqueries, the CRDB node could panic while walking the statement due to a missing nil check. No release note since this only affected an experimental feature. fixes #130822 Release note: None 130896: tree: disallow creating partial index while table is used in LDR r=rafiss a=rafiss Epic: CRDB-38974 Release note: None Co-authored-by: Rafi Shamim <[email protected]>
cockroachdb · Sep 17, 2024 · 832ba88 · 832ba88
3 parents 40b4ad8 + e248fcc + 3516990
commit 832ba88
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 8 deletions.
diff --git a/pkg/sql/sem/tree/schema_helpers.go b/pkg/sql/sem/tree/schema_helpers.go
@@ -40,9 +40,9 @@ func IsSetOrResetSchemaLocked(n Statement) bool {
 func IsAllowedLDRSchemaChange(n Statement) bool {
 	switch s := n.(type) {
 	case *CreateIndex:
-		// Only allow non-unique indexes to be created. A unique index on a
-		// destination table could cause inserts to fail.
-		return !s.Unique
+		// Only allow non-unique and non-partial indexes to be created. A unique or
+		// partial index on a destination table could cause inserts to fail.
+		return !s.Unique && s.Predicate == nil
 	case *DropIndex:
 		return true
 	}

diff --git a/pkg/sql/sem/tree/schema_helpers_test.go b/pkg/sql/sem/tree/schema_helpers_test.go
@@ -35,6 +35,10 @@ func TestIsAllowedLDRSchemaChange(t *testing.T) {
 			stmt:      "CREATE UNIQUE INDEX idx ON t (a)",
 			isAllowed: false,
 		},
+		{
+			stmt:      "CREATE INDEX idx ON t (a) WHERE a > 10",
+			isAllowed: false,
+		},
 		{
 			stmt:      "DROP INDEX idx",
 			isAllowed: true,

diff --git a/pkg/sql/sem/tree/walk.go b/pkg/sql/sem/tree/walk.go
@@ -1152,12 +1152,14 @@ func (n *ShowFingerprints) copyNode() *ShowFingerprints {
 // walkStmt is part of the walkableStmt interface.
 func (n *ShowFingerprints) walkStmt(v Visitor) Statement {
 	ret := n
-	ts, changed := walkTenantSpec(v, n.TenantSpec)
-	if changed {
-		if ret == n {
-			ret = n.copyNode()
+	if n.TenantSpec != nil {
+		ts, changed := walkTenantSpec(v, n.TenantSpec)
+		if changed {
+			if ret == n {
+				ret = n.copyNode()
+			}
+			ret.TenantSpec = ts
 		}
-		ret.TenantSpec = ts
 	}
 	if n.Options.StartTimestamp != nil {
 		e, changed := WalkExpr(v, n.Options.StartTimestamp)

diff --git a/pkg/testutils/sqlutils/pretty.go b/pkg/testutils/sqlutils/pretty.go
@@ -29,6 +29,15 @@ func VerifyStatementPrettyRoundtrip(t *testing.T, sql string) {
 	for i := range stmts {
 		origStmt := stmts[i].AST
 		verifyStatementPrettyRoundTrip(t, sql, origStmt, false /* plpgsql */)
+
+		// Verify that the AST can be walked.
+		if _, err := tree.SimpleStmtVisit(
+			origStmt,
+			func(expr tree.Expr) (recurse bool, newExpr tree.Expr, err error) { return },
+		); err != nil {
+			t.Fatalf("cannot walk stmt %s %v", stmts[i].SQL, err)
+		}
+
 	}
 }