Skip to content

Commit 9188ab7

Browse files
rafissrafiss
committed
sql: clean up comments in legacy DROP TYPE
The cleanup logic was not removing comments on types. 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. Release note: None
1 parent 42ccf7b commit 9188ab7

File tree

3 files changed

+135
-23
lines changed

3 files changed

+135
-23
lines changed

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

Lines changed: 72 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
2424
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
2525
"github.com/cockroachdb/cockroach/pkg/util/log"
26+
"github.com/cockroachdb/cockroach/pkg/util/randutil"
2627
"github.com/cockroachdb/cockroach/pkg/workload"
2728
"github.com/cockroachdb/cockroach/pkg/workload/histogram"
2829
_ "github.com/cockroachdb/cockroach/pkg/workload/schemachange"
@@ -36,6 +37,7 @@ func TestWorkload(t *testing.T) {
3637
skip.UnderDeadlock(t, "test connections can be too slow under expensive configs")
3738
skip.UnderRace(t, "test connections can be too slow under expensive configs")
3839

40+
rng, _ := randutil.NewTestRand()
3941
scope := log.Scope(t)
4042
defer scope.Close(t)
4143
dir := scope.GetDirectory()
@@ -70,31 +72,78 @@ func TestWorkload(t *testing.T) {
7072
require.NoError(t, os.WriteFile(fmt.Sprintf("%s/%s.rows", dir, name), []byte(sqlutils.MatrixToStr(mat)), 0666))
7173
}
7274

73-
// Grab a backup, dump the namespace and descriptor tables upon failure.
75+
// Reusable validation function.
76+
findInvalidObjects := func() {
77+
t.Helper()
78+
var (
79+
id int
80+
databaseName string
81+
schemaName string
82+
objName string
83+
objError string
84+
)
85+
numInvalidObjects := 0
86+
rows, err := tdb.DB.QueryContext(ctx, `SELECT id, database_name, schema_name, obj_name, error FROM "".crdb_internal.invalid_objects`)
87+
if err != nil {
88+
t.Fatal(err)
89+
}
90+
for rows.Next() {
91+
numInvalidObjects++
92+
if err := rows.Scan(&id, &databaseName, &schemaName, &objName, &objError); err != nil {
93+
t.Fatal(err)
94+
}
95+
t.Logf(
96+
"invalid object found: id: %d, database_name: %s, schema_name: %s, obj_name: %s, error: %s",
97+
id, databaseName, schemaName, objName, objError,
98+
)
99+
}
100+
if err := rows.Err(); err != nil {
101+
t.Fatal(err)
102+
}
103+
if numInvalidObjects > 0 {
104+
t.Errorf("found %d invalid objects", numInvalidObjects)
105+
}
106+
}
107+
74108
defer func() {
75-
if !t.Failed() {
76-
return
109+
// Run validation before dropping the database.
110+
findInvalidObjects()
111+
112+
// Only take a backup if the test failed.
113+
if t.Failed() {
114+
// Dump namespace and descriptor in their raw format. This is useful for
115+
// processing results with some degree of scripting.
116+
dumpRows("namespace", tdb.Query(t, `SELECT * FROM system.namespace`))
117+
dumpRows("descriptor", tdb.Query(t, "SELECT id, encode(descriptor, 'hex') FROM system.descriptor"))
118+
// Dump out a more human readable version of the above as well to allow for
119+
// easy debugging by hand.
120+
// NB: A LEFT JOIN is used here because not all descriptors (looking at you
121+
// functions) have namespace entries.
122+
dumpRows("ns-desc-json", tdb.Query(t, `
123+
SELECT
124+
"parentID",
125+
"parentSchemaID",
126+
descriptor.id,
127+
name,
128+
crdb_internal.pb_to_json('cockroach.sql.sqlbase.Descriptor', descriptor)
129+
FROM system.descriptor
130+
LEFT JOIN system.namespace ON namespace.id = descriptor.id
131+
`))
132+
tdb.Exec(t, "BACKUP DATABASE schemachange INTO 'nodelocal://1/backup'")
133+
t.Logf("backup, tracing data, and system table dumps in %s", dir)
134+
}
135+
136+
// Drop the database and run validation again. Test DROP DATABASE behavior
137+
// with legacy schema changer 50% of the time.
138+
schemaChangerSetting := "on"
139+
if rng.Float32() < 0.5 {
140+
schemaChangerSetting = "off"
77141
}
78-
// Dump namespace and descriptor in their raw format. This is useful for
79-
// processing results with some degree of scripting.
80-
dumpRows("namespace", tdb.Query(t, `SELECT * FROM system.namespace`))
81-
dumpRows("descriptor", tdb.Query(t, "SELECT id, encode(descriptor, 'hex') FROM system.descriptor"))
82-
// Dump out a more human readable version of the above as well to allow for
83-
// easy debugging by hand.
84-
// NB: A LEFT JOIN is used here because not all descriptors (looking at you
85-
// functions) have namespace entries.
86-
dumpRows("ns-desc-json", tdb.Query(t, `
87-
SELECT
88-
"parentID",
89-
"parentSchemaID",
90-
descriptor.id,
91-
name,
92-
crdb_internal.pb_to_json('cockroach.sql.sqlbase.Descriptor', descriptor)
93-
FROM system.descriptor
94-
LEFT JOIN system.namespace ON namespace.id = descriptor.id
95-
`))
96-
tdb.Exec(t, "BACKUP DATABASE schemachange INTO 'nodelocal://1/backup'")
97-
t.Logf("backup, tracing data, and system table dumps in %s", dir)
142+
t.Logf("running DROP with use_declarative_schema_changer = %s", schemaChangerSetting)
143+
tdb.Exec(t, "SET use_declarative_schema_changer = $1", schemaChangerSetting)
144+
tdb.Exec(t, "DROP DATABASE schemachange CASCADE")
145+
tdb.Exec(t, "RESET use_declarative_schema_changer")
146+
findInvalidObjects()
98147
}()
99148

100149
pgURL, cleanup := pgurlutils.PGUrl(t, tc.Server(0).AdvSQLAddr(), t.Name(), url.User("testuser"))

‎pkg/sql/drop_type.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"fmt"
1111

1212
"github.com/cockroachdb/cockroach/pkg/sql/catalog"
13+
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
1314
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
1415
"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
1516
"github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc"
@@ -291,6 +292,11 @@ func (p *planner) dropTypeImpl(
291292
return err
292293
}
293294

295+
// Delete any comments associated with this type.
296+
if err := p.deleteComment(ctx, typeDesc.ID, 0, catalogkeys.TypeCommentType); err != nil {
297+
return err
298+
}
299+
294300
// For this type descriptor, delete queued schema change jobs from the job
295301
// cache. If the job had already been scheduled, we would also need to mark
296302
// these jobs as successful, but that should never happen for types.

‎pkg/sql/logictest/testdata/logic_test/comment_on‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -578,3 +578,60 @@ SELECT * FROM crdb_internal.invalid_objects ORDER BY id;
578578
----
579579

580580
subtest end
581+
582+
# Test for issue #146516: Ensure type comments are cleaned up when dropping types
583+
# This test verifies that COMMENT ON TYPE followed by DROP DATABASE CASCADE
584+
# properly cleans up orphaned comments to avoid "invalid objects" errors.
585+
subtest type_comment_cleanup_on_drop_database_cascade
586+
587+
statement ok
588+
CREATE DATABASE test_db
589+
590+
statement ok
591+
USE test_db
592+
593+
statement ok
594+
CREATE TYPE roach_type AS ENUM ('option1', 'option2')
595+
596+
# Skip for legacy schema changer as COMMENT ON TYPE is not supported
597+
skipif config local-legacy-schema-changer
598+
statement ok
599+
COMMENT ON TYPE roach_type IS 'This is a test comment on a type'
600+
601+
skipif config local-legacy-schema-changer
602+
query TTTT colnames
603+
SHOW TYPES WITH COMMENT
604+
----
605+
schema name owner comment
606+
public roach_type root This is a test comment on a type
607+
608+
# Now drop the database with CASCADE, which should clean up type comments
609+
statement ok
610+
USE defaultdb
611+
612+
let $schema_changer_state
613+
SHOW use_declarative_schema_changer
614+
615+
statement ok
616+
SET use_declarative_schema_changer = 'off'
617+
618+
statement ok
619+
DROP DATABASE test_db CASCADE
620+
621+
# Restore the schema changer state back.
622+
statement ok
623+
SET use_declarative_schema_changer = $schema_changer_state
624+
625+
# Check that no invalid objects exist - this should be empty
626+
# The issue was that type comments were not being cleaned up,
627+
# leaving orphaned entries in system.comments
628+
query ITTTT
629+
SELECT id, database_name, schema_name, obj_name, error FROM "".crdb_internal.invalid_objects
630+
----
631+
632+
# Verify the database is actually gone
633+
query T
634+
SELECT database_name FROM [SHOW DATABASES] WHERE database_name = 'test_db'
635+
----
636+
637+
subtest end

0 commit comments

Comments
 (0)