Merge #147173

147173: sql: clean up comments in legacy DROP TYPE r=rafiss a=rafiss

The cleanup logic was not removing comments on types. This was caught by the new validation added in #146213.

This patch adds a logic test that reveals the bug, and also adds validation to TestWorkload. This validation is already in the schemachange/random-load roachtest. Doing it here too can help catch bugs earlier.

fixes #146516
fixes #146913
fixes #146793

Release note: None

Co-authored-by: Rafi Shamim <[email protected]>

Author: craig[bot]

pkg/ccl/testccl/workload/schemachange/schema_change_external_test.go

@@ -23,9 +23,10 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/randutil"
 	"github.com/cockroachdb/cockroach/pkg/workload"
 	"github.com/cockroachdb/cockroach/pkg/workload/histogram"
-	_ "github.com/cockroachdb/cockroach/pkg/workload/schemachange"
+	"github.com/cockroachdb/cockroach/pkg/workload/schemachange"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/sync/errgroup"
 )
@@ -36,6 +37,7 @@ func TestWorkload(t *testing.T) {
 	skip.UnderDeadlock(t, "test connections can be too slow under expensive configs")
 	skip.UnderRace(t, "test connections can be too slow under expensive configs")
 
+	rng, _ := randutil.NewTestRand()
 	scope := log.Scope(t)
 	defer scope.Close(t)
 	dir := scope.GetDirectory()
@@ -56,7 +58,8 @@ func TestWorkload(t *testing.T) {
 		workload.Opser
 		workload.Flagser
 	})
-	tdb := sqlutils.MakeSQLRunner(tc.ServerConn(0))
+	db := tc.ServerConn(0)
+	tdb := sqlutils.MakeSQLRunner(db)
 	reg := histogram.NewRegistry(20*time.Second, m.Name)
 	tdb.Exec(t, "CREATE USER testuser")
 	tdb.Exec(t, "CREATE DATABASE schemachange")
@@ -70,31 +73,61 @@ func TestWorkload(t *testing.T) {
 		require.NoError(t, os.WriteFile(fmt.Sprintf("%s/%s.rows", dir, name), []byte(sqlutils.MatrixToStr(mat)), 0666))
 	}
 
-	// Grab a backup, dump the namespace and descriptor tables upon failure.
+	findInvalidObjects := func() {
+		t.Helper()
+		invalidObjects, err := schemachange.ValidateInvalidObjects(ctx, db)
+		if err != nil {
+			t.Fatal(err)
+		}
+		for _, obj := range invalidObjects {
+			t.Logf(
+				"invalid object found: id: %d, database_name: %s, schema_name: %s, obj_name: %s, error: %v",
+				obj.ID, obj.DatabaseName, obj.SchemaName, obj.ObjName, obj.Error,
+			)
+		}
+		if len(invalidObjects) > 0 {
+			t.Errorf("found %d invalid objects", len(invalidObjects))
+		}
+	}
+
 	defer func() {
-		if !t.Failed() {
-			return
+		// Run validation before dropping the database.
+		findInvalidObjects()
+
+		// Only take a backup if the test failed.
+		if t.Failed() {
+			// Dump namespace and descriptor in their raw format. This is useful for
+			// processing results with some degree of scripting.
+			dumpRows("namespace", tdb.Query(t, `SELECT * FROM system.namespace`))
+			dumpRows("descriptor", tdb.Query(t, "SELECT id, encode(descriptor, 'hex') FROM system.descriptor"))
+			// Dump out a more human readable version of the above as well to allow for
+			// easy debugging by hand.
+			// NB: A LEFT JOIN is used here because not all descriptors (looking at you
+			// functions) have namespace entries.
+			dumpRows("ns-desc-json", tdb.Query(t, `
+				SELECT
+					"parentID",
+					"parentSchemaID",
+					descriptor.id,
+					name,
+					crdb_internal.pb_to_json('cockroach.sql.sqlbase.Descriptor', descriptor)
+					FROM system.descriptor
+					LEFT JOIN system.namespace ON namespace.id = descriptor.id
+			`))
+			tdb.Exec(t, "BACKUP DATABASE schemachange INTO 'nodelocal://1/backup'")
+			t.Logf("backup, tracing data, and system table dumps in %s", dir)
+		}
+
+		// Drop the database and run validation again. Test DROP DATABASE behavior
+		// with legacy schema changer 50% of the time.
+		schemaChangerSetting := "on"
+		if rng.Float32() < 0.5 {
+			schemaChangerSetting = "off"
 		}
-		// Dump namespace and descriptor in their raw format. This is useful for
-		// processing results with some degree of scripting.
-		dumpRows("namespace", tdb.Query(t, `SELECT * FROM system.namespace`))
-		dumpRows("descriptor", tdb.Query(t, "SELECT id, encode(descriptor, 'hex') FROM system.descriptor"))
-		// Dump out a more human readable version of the above as well to allow for
-		// easy debugging by hand.
-		// NB: A LEFT JOIN is used here because not all descriptors (looking at you
-		// functions) have namespace entries.
-		dumpRows("ns-desc-json", tdb.Query(t, `
-			SELECT
-				"parentID",
-				"parentSchemaID",
-				descriptor.id,
-				name,
-				crdb_internal.pb_to_json('cockroach.sql.sqlbase.Descriptor', descriptor)
-				FROM system.descriptor
-				LEFT JOIN system.namespace ON namespace.id = descriptor.id
-		`))
-		tdb.Exec(t, "BACKUP DATABASE schemachange INTO 'nodelocal://1/backup'")
-		t.Logf("backup, tracing data, and system table dumps in %s", dir)
+		t.Logf("running DROP with use_declarative_schema_changer = %s", schemaChangerSetting)
+		tdb.Exec(t, "SET use_declarative_schema_changer = $1", schemaChangerSetting)
+		tdb.Exec(t, "DROP DATABASE schemachange CASCADE")
+		findInvalidObjects()
 	}()
 
 	pgURL, cleanup := pgurlutils.PGUrl(t, tc.Server(0).AdvSQLAddr(), t.Name(), url.User("testuser"))

pkg/cmd/roachtest/tests/BUILD.bazel

@@ -304,6 +304,7 @@ go_library(
         "//pkg/workload/debug",
         "//pkg/workload/histogram",
         "//pkg/workload/histogram/exporter",
+        "//pkg/workload/schemachange",
         "//pkg/workload/tpcc",
         "//pkg/workload/tpcds",
         "//pkg/workload/tpch",

pkg/cmd/roachtest/tests/schemachange_random_load.go

@@ -9,6 +9,7 @@ import (
 	"context"
 	gosql "database/sql"
 	"fmt"
+	"math/rand/v2"
 	"path/filepath"
 
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster"
@@ -18,6 +19,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/spec"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/install"
+	"github.com/cockroachdb/cockroach/pkg/workload/schemachange"
 )
 
 const (
@@ -57,33 +59,18 @@ func runSchemaChangeRandomLoad(
 	ctx context.Context, t test.Test, c cluster.Cluster, maxOps, concurrency int,
 ) {
 	validate := func(db *gosql.DB) {
-		var (
-			id           int
-			databaseName string
-			schemaName   string
-			objName      string
-			objError     string
-		)
-		numInvalidObjects := 0
-		rows, err := db.QueryContext(ctx, `SELECT id, database_name, schema_name, obj_name, error FROM crdb_internal.invalid_objects`)
+		invalidObjects, err := schemachange.ValidateInvalidObjects(ctx, db)
 		if err != nil {
 			t.Fatal(err)
 		}
-		for rows.Next() {
-			numInvalidObjects++
-			if err := rows.Scan(&id, &databaseName, &schemaName, &objName, &objError); err != nil {
-				t.Fatal(err)
-			}
+		for _, obj := range invalidObjects {
 			t.L().Errorf(
-				"invalid object found: id: %d, database_name: %s, schema_name: %s, obj_name: %s, error: %s",
-				id, databaseName, schemaName, objName, objError,
+				"invalid object found: id: %d, database_name: %s, schema_name: %s, obj_name: %s, error: %v",
+				obj.ID, obj.DatabaseName, obj.SchemaName, obj.ObjName, obj.Error,
 			)
 		}
-		if err := rows.Err(); err != nil {
-			t.Fatal(err)
-		}
-		if numInvalidObjects > 0 {
-			t.Fatalf("found %d invalid objects", numInvalidObjects)
+		if len(invalidObjects) > 0 {
+			t.Fatalf("found %d invalid objects", len(invalidObjects))
 		}
 	}
 	loadNode := c.Node(1)
@@ -142,8 +129,16 @@ func runSchemaChangeRandomLoad(
 
 	t.Status("performing validation after workload")
 	validate(db)
-	t.Status("dropping database")
-	_, err = db.ExecContext(ctx, `USE defaultdb; DROP DATABASE schemachange CASCADE;`)
+	schemaChangerSetting := "on"
+	if rand.Float32() < 0.5 {
+		schemaChangerSetting = "off"
+	}
+	t.Status(fmt.Sprintf("dropping database with use_declarative_schema_changer = %s", schemaChangerSetting))
+	_, err = db.ExecContext(ctx, "SET use_declarative_schema_changer = $1", schemaChangerSetting)
+	if err != nil {
+		t.Fatal(err)
+	}
+	_, err = db.ExecContext(ctx, "DROP DATABASE schemachange CASCADE")
 	if err != nil {
 		t.Fatal(err)
 	}

pkg/sql/drop_type.go

@@ -10,6 +10,7 @@ import (
 	"fmt"
 
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc"
@@ -291,6 +292,11 @@ func (p *planner) dropTypeImpl(
 		return err
 	}
 
+	// Delete any comments associated with this type.
+	if err := p.deleteComment(ctx, typeDesc.ID, 0, catalogkeys.TypeCommentType); err != nil {
+		return err
+	}
+
 	// For this type descriptor, delete queued schema change jobs from the job
 	// cache. If the job had already been scheduled, we would also need to mark
 	// these jobs as successful, but that should never happen for types.

pkg/sql/logictest/testdata/logic_test/comment_on

@@ -578,3 +578,60 @@ SELECT * FROM crdb_internal.invalid_objects ORDER BY id;
 ----
 
 subtest end
+
+# Test for issue #146516: Ensure type comments are cleaned up when dropping types
+# This test verifies that COMMENT ON TYPE followed by DROP DATABASE CASCADE
+# properly cleans up orphaned comments to avoid "invalid objects" errors.
+subtest type_comment_cleanup_on_drop_database_cascade
+
+statement ok
+CREATE DATABASE test_db
+
+statement ok
+USE test_db
+
+statement ok
+CREATE TYPE roach_type AS ENUM ('option1', 'option2')
+
+# Skip for legacy schema changer as COMMENT ON TYPE is not supported
+skipif config local-legacy-schema-changer
+statement ok
+COMMENT ON TYPE roach_type IS 'This is a test comment on a type'
+
+skipif config local-legacy-schema-changer
+query TTTT colnames
+SHOW TYPES WITH COMMENT
+----
+schema  name        owner  comment
+public  roach_type  root   This is a test comment on a type
+
+# Now drop the database with CASCADE, which should clean up type comments
+statement ok
+USE defaultdb
+
+let $schema_changer_state
+SHOW use_declarative_schema_changer
+
+statement ok
+SET use_declarative_schema_changer = 'off'
+
+statement ok
+DROP DATABASE test_db CASCADE
+
+# Restore the schema changer state back.
+statement ok
+SET use_declarative_schema_changer = $schema_changer_state
+
+# Check that no invalid objects exist - this should be empty
+# The issue was that type comments were not being cleaned up,
+# leaving orphaned entries in system.comments
+query ITTTT
+SELECT id, database_name, schema_name, obj_name, error FROM "".crdb_internal.invalid_objects
+----
+
+# Verify the database is actually gone
+query T
+SELECT database_name FROM [SHOW DATABASES] WHERE database_name = 'test_db'
+----
+
+subtest end

pkg/workload/schemachange/BUILD.bazel

@@ -14,6 +14,7 @@ go_library(
         "schemachange.go",
         "tracing.go",
         "type_resolver.go",
+        "validation.go",
         "watch_dog.go",
         "workload_result.go",
         ":gen-optype-stringer",  # keep

pkg/workload/schemachange/validation.go

@@ -0,0 +1,51 @@
+// Copyright 2025 The Cockroach Authors.
+//
+// Use of this software is governed by the CockroachDB Software License
+// included in the /LICENSE file.
+
+package schemachange
+
+import (
+	"context"
+	gosql "database/sql"
+
+	"github.com/cockroachdb/errors"
+)
+
+// InvalidObject represents an invalid database object found during validation.
+type InvalidObject struct {
+	ID           int
+	DatabaseName string
+	SchemaName   string
+	ObjName      string
+	Error        string
+}
+
+// ValidateInvalidObjects checks for invalid objects in the database by querying
+// crdb_internal.invalid_objects. It returns a slice of InvalidObject structs
+// representing any invalid objects found, or an error if the query fails.
+//
+// This function is useful for validating that schema change operations haven't
+// left the database in an inconsistent state with orphaned or invalid objects.
+func ValidateInvalidObjects(ctx context.Context, db *gosql.DB) ([]InvalidObject, error) {
+	query := `SELECT id, database_name, schema_name, obj_name, error FROM crdb_internal.invalid_objects`
+	rows, err := db.QueryContext(ctx, query)
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to query invalid objects")
+	}
+
+	var invalidObjects []InvalidObject
+	for rows.Next() {
+		var obj InvalidObject
+		if err := rows.Scan(&obj.ID, &obj.DatabaseName, &obj.SchemaName, &obj.ObjName, &obj.Error); err != nil {
+			return nil, errors.Wrapf(err, "failed to scan invalid object row")
+		}
+		invalidObjects = append(invalidObjects, obj)
+	}
+
+	if err := rows.Err(); err != nil {
+		return nil, errors.Wrapf(err, "error iterating invalid objects")
+	}
+
+	return invalidObjects, nil
+}