From d4620be292c5fdc26956b86571ba31eaedff9505 Mon Sep 17 00:00:00 2001 From: Chengxiong Ruan Date: Thu, 10 Nov 2022 18:32:42 +0000 Subject: [PATCH] sql: drop comment when drop database in legacy schema changer. Looks like we accidentally drop the logic to delete database comment when we refactor metadata updater in #83592. Epic: None. Release note (sql change): Fixed a bug in legacy schema changer that comment was not dropped together with database. --- pkg/sql/comment_on_column_test.go | 262 +++++++++++++------------- pkg/sql/comment_on_constraint_test.go | 137 +++++++------- pkg/sql/comment_on_database_test.go | 119 ++++++------ pkg/sql/comment_on_index_test.go | 167 ++++++++-------- pkg/sql/comment_on_schema_test.go | 119 ++++++------ pkg/sql/comment_on_table_test.go | 119 ++++++------ pkg/sql/drop_database.go | 9 + 7 files changed, 457 insertions(+), 475 deletions(-) diff --git a/pkg/sql/comment_on_column_test.go b/pkg/sql/comment_on_column_test.go index fdb264c88460..75f2b84cdaa6 100644 --- a/pkg/sql/comment_on_column_test.go +++ b/pkg/sql/comment_on_column_test.go @@ -21,86 +21,82 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/errors" + "github.com/stretchr/testify/require" ) func TestCommentOnColumn(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - params, _ := tests.CreateTestServerParams() - s, db, _ := serverutils.StartServer(t, params) - defer s.Stopper().Stop(context.Background()) - - if _, err := db.Exec(` + runCommentOnTests(t, func(db *gosql.DB) { + if _, err := db.Exec(` CREATE DATABASE d; SET DATABASE = d; CREATE TABLE t (c1 INT, c2 INT, c3 INT); `); err != nil { - t.Fatal(err) - } - - testCases := []struct { - exec string - query string - expect gosql.NullString - }{ - { - `COMMENT ON COLUMN t.c1 IS 'foo'`, - `SELECT col_description(attrelid, attnum) FROM pg_attribute WHERE attrelid = 't'::regclass AND attname = 'c1'`, - gosql.NullString{String: `foo`, Valid: true}, - }, - { - `TRUNCATE t`, - `SELECT col_description(attrelid, attnum) FROM pg_attribute WHERE attrelid = 't'::regclass AND attname = 'c1'`, - gosql.NullString{String: `foo`, Valid: true}, - }, - { - `ALTER TABLE t RENAME COLUMN c1 TO c1_1`, - `SELECT col_description(attrelid, attnum) FROM pg_attribute WHERE attrelid = 't'::regclass AND attname = 'c1_1'`, - gosql.NullString{String: `foo`, Valid: true}, - }, - { - `COMMENT ON COLUMN t.c1_1 IS NULL`, - `SELECT col_description(attrelid, attnum) FROM pg_attribute WHERE attrelid = 't'::regclass AND attname = 'c1_1'`, - gosql.NullString{Valid: false}, - }, - { - `COMMENT ON COLUMN t.c3 IS 'foo'`, - `SELECT col_description(attrelid, attnum) FROM pg_attribute WHERE attrelid = 't'::regclass AND attname = 'c3'`, - gosql.NullString{String: `foo`, Valid: true}, - }, - { - `ALTER TABLE t DROP COLUMN c2`, - `SELECT col_description(attrelid, attnum) FROM pg_attribute WHERE attrelid = 't'::regclass AND attname = 'c3'`, - gosql.NullString{String: `foo`, Valid: true}, - }, - } - - for _, tc := range testCases { - if _, err := db.Exec(tc.exec); err != nil { t.Fatal(err) } - row := db.QueryRow(tc.query) - var comment gosql.NullString - if err := row.Scan(&comment); err != nil { - t.Fatal(err) + testCases := []struct { + exec string + query string + expect gosql.NullString + }{ + { + `COMMENT ON COLUMN t.c1 IS 'foo'`, + `SELECT col_description(attrelid, attnum) FROM pg_attribute WHERE attrelid = 't'::regclass AND attname = 'c1'`, + gosql.NullString{String: `foo`, Valid: true}, + }, + { + `TRUNCATE t`, + `SELECT col_description(attrelid, attnum) FROM pg_attribute WHERE attrelid = 't'::regclass AND attname = 'c1'`, + gosql.NullString{String: `foo`, Valid: true}, + }, + { + `ALTER TABLE t RENAME COLUMN c1 TO c1_1`, + `SELECT col_description(attrelid, attnum) FROM pg_attribute WHERE attrelid = 't'::regclass AND attname = 'c1_1'`, + gosql.NullString{String: `foo`, Valid: true}, + }, + { + `COMMENT ON COLUMN t.c1_1 IS NULL`, + `SELECT col_description(attrelid, attnum) FROM pg_attribute WHERE attrelid = 't'::regclass AND attname = 'c1_1'`, + gosql.NullString{Valid: false}, + }, + { + `COMMENT ON COLUMN t.c3 IS 'foo'`, + `SELECT col_description(attrelid, attnum) FROM pg_attribute WHERE attrelid = 't'::regclass AND attname = 'c3'`, + gosql.NullString{String: `foo`, Valid: true}, + }, + { + `ALTER TABLE t DROP COLUMN c2`, + `SELECT col_description(attrelid, attnum) FROM pg_attribute WHERE attrelid = 't'::regclass AND attname = 'c3'`, + gosql.NullString{String: `foo`, Valid: true}, + }, } - if tc.expect != comment { - t.Fatalf("expected comment %v, got %v", tc.expect, comment) + + for _, tc := range testCases { + if _, err := db.Exec(tc.exec); err != nil { + t.Fatal(err) + } + + row := db.QueryRow(tc.query) + var comment gosql.NullString + if err := row.Scan(&comment); err != nil { + t.Fatal(err) + } + if tc.expect != comment { + t.Fatalf("expected comment %v, got %v", tc.expect, comment) + } } - } + }) } func TestCommentOnColumnTransaction(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - params, _ := tests.CreateTestServerParams() - s, db, _ := serverutils.StartServer(t, params) - defer s.Stopper().Stop(context.Background()) - - if _, err := db.Exec(` + runCommentOnTests(t, func(db *gosql.DB) { + if _, err := db.Exec(` CREATE DATABASE d; SET DATABASE = d; CREATE TABLE t (c INT); @@ -109,118 +105,130 @@ func TestCommentOnColumnTransaction(t *testing.T) { COMMENT ON COLUMN t.x IS 'foo'; COMMIT; `); err != nil { - t.Fatal(err) - } + t.Fatal(err) + } + }) } func TestCommentOnColumnWhenDropTable(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - params, _ := tests.CreateTestServerParams() - s, db, _ := serverutils.StartServer(t, params) - defer s.Stopper().Stop(context.Background()) - - if _, err := db.Exec(` + runCommentOnTests(t, func(db *gosql.DB) { + if _, err := db.Exec(` CREATE DATABASE d; SET DATABASE = d; CREATE TABLE t (c INT); `); err != nil { - t.Fatal(err) - } - - if _, err := db.Exec(`COMMENT ON COLUMN t.c IS 'foo'`); err != nil { - t.Fatal(err) - } + t.Fatal(err) + } - if _, err := db.Exec(`DROP TABLE t`); err != nil { - t.Fatal(err) - } + if _, err := db.Exec(`COMMENT ON COLUMN t.c IS 'foo'`); err != nil { + t.Fatal(err) + } - row := db.QueryRow(`SELECT comment FROM system.comments LIMIT 1`) - var comment string - err := row.Scan(&comment) - if !errors.Is(err, gosql.ErrNoRows) { - if err != nil { + if _, err := db.Exec(`DROP TABLE t`); err != nil { t.Fatal(err) } - t.Fatal("comment remain") - } + row := db.QueryRow(`SELECT comment FROM system.comments LIMIT 1`) + var comment string + err := row.Scan(&comment) + if !errors.Is(err, gosql.ErrNoRows) { + if err != nil { + t.Fatal(err) + } + + t.Fatal("comment remain") + } + }) } func TestCommentOnColumnWhenDropColumn(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - params, _ := tests.CreateTestServerParams() - s, db, _ := serverutils.StartServer(t, params) - defer s.Stopper().Stop(context.Background()) - - if _, err := db.Exec(` + runCommentOnTests(t, func(db *gosql.DB) { + if _, err := db.Exec(` CREATE DATABASE d; SET DATABASE = d; CREATE TABLE t (c INT); `); err != nil { - t.Fatal(err) - } - - if _, err := db.Exec(`COMMENT ON COLUMN t.c IS 'foo'`); err != nil { - t.Fatal(err) - } + t.Fatal(err) + } - if _, err := db.Exec(`ALTER TABLE t DROP COLUMN c`); err != nil { - t.Fatal(err) - } + if _, err := db.Exec(`COMMENT ON COLUMN t.c IS 'foo'`); err != nil { + t.Fatal(err) + } - row := db.QueryRow(`SELECT comment FROM system.comments LIMIT 1`) - var comment string - err := row.Scan(&comment) - if !errors.Is(err, gosql.ErrNoRows) { - if err != nil { + if _, err := db.Exec(`ALTER TABLE t DROP COLUMN c`); err != nil { t.Fatal(err) } - t.Fatal("comment remaining in system.comments despite drop") - } + row := db.QueryRow(`SELECT comment FROM system.comments LIMIT 1`) + var comment string + err := row.Scan(&comment) + if !errors.Is(err, gosql.ErrNoRows) { + if err != nil { + t.Fatal(err) + } + + t.Fatal("comment remaining in system.comments despite drop") + } + }) } func TestCommentOnAlteredColumn(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - params, _ := tests.CreateTestServerParams() - s, db, _ := serverutils.StartServer(t, params) - defer s.Stopper().Stop(context.Background()) - expectedComment := "expected comment" + runCommentOnTests(t, func(db *gosql.DB) { + expectedComment := "expected comment" - if _, err := db.Exec(` + if _, err := db.Exec(` CREATE DATABASE d; SET DATABASE = d; SET enable_experimental_alter_column_type_general = true; CREATE TABLE t (c INT); `); err != nil { - t.Fatal(err) - } + t.Fatal(err) + } - if _, err := db.Exec(`COMMENT ON COLUMN t.c IS 'first comment'`); err != nil { - t.Fatal(err) - } + if _, err := db.Exec(`COMMENT ON COLUMN t.c IS 'first comment'`); err != nil { + t.Fatal(err) + } - if _, err := db.Exec(`ALTER TABLE t ALTER COLUMN c TYPE character varying;`); err != nil { - t.Fatal(err) - } - if _, err := db.Exec( - fmt.Sprintf(`COMMENT ON COLUMN t.c IS '%s'`, expectedComment)); err != nil { - t.Fatal(err) - } - row := db.QueryRow(`SELECT comment FROM system.comments LIMIT 1`) - var comment string - if err := row.Scan(&comment); err != nil { - t.Fatal(err) - } + if _, err := db.Exec(`ALTER TABLE t ALTER COLUMN c TYPE character varying;`); err != nil { + t.Fatal(err) + } + if _, err := db.Exec( + fmt.Sprintf(`COMMENT ON COLUMN t.c IS '%s'`, expectedComment)); err != nil { + t.Fatal(err) + } + row := db.QueryRow(`SELECT comment FROM system.comments LIMIT 1`) + var comment string + if err := row.Scan(&comment); err != nil { + t.Fatal(err) + } + + if expectedComment != comment { + t.Fatalf("expected comment %v, got %v", expectedComment, comment) + } + }) +} - if expectedComment != comment { - t.Fatalf("expected comment %v, got %v", expectedComment, comment) +func runCommentOnTests(t *testing.T, testFunc func(db *gosql.DB)) { + for _, setupQuery := range []string{ + `SET use_declarative_schema_changer = 'on'`, + `SET use_declarative_schema_changer = 'off'`, + } { + func() { + params, _ := tests.CreateTestServerParams() + s, db, _ := serverutils.StartServer(t, params) + defer s.Stopper().Stop(context.Background()) + _, err := db.Exec(setupQuery) + require.NoError(t, err) + testFunc(db) + }() } } diff --git a/pkg/sql/comment_on_constraint_test.go b/pkg/sql/comment_on_constraint_test.go index 4ea597c3d18e..a56c3a8fad1a 100644 --- a/pkg/sql/comment_on_constraint_test.go +++ b/pkg/sql/comment_on_constraint_test.go @@ -11,12 +11,9 @@ package sql_test import ( - "context" gosql "database/sql" "testing" - "github.com/cockroachdb/cockroach/pkg/sql/tests" - "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" ) @@ -25,11 +22,8 @@ func TestCommentOnConstraint(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - params, _ := tests.CreateTestServerParams() - s, db, _ := serverutils.StartServer(t, params) - defer s.Stopper().Stop(context.Background()) - - if _, err := db.Exec(` + runCommentOnTests(t, func(db *gosql.DB) { + if _, err := db.Exec(` CREATE DATABASE d; SET DATABASE = d; CREATE TABLE t ( a int UNIQUE, b numeric CONSTRAINT positive_price CHECK (b > 0), c int CHECK (b > c), CONSTRAINT pkey PRIMARY KEY (a,c)); @@ -37,70 +31,71 @@ func TestCommentOnConstraint(t *testing.T) { CREATE SCHEMA s; CREATE TABLE s.t ( a int UNIQUE, b numeric CONSTRAINT positive_price CHECK (b > 0), c int CHECK (b > c), CONSTRAINT pkey PRIMARY KEY (a,c)); `); err != nil { - t.Fatal(err) - } + t.Fatal(err) + } - testCases := []struct { - exec string - query string - expect gosql.NullString - }{ - { - `COMMENT ON CONSTRAINT t_a_key ON t IS 'unique_comment'`, - `SELECT obj_description(oid, 'pg_constraint') FROM pg_constraint WHERE conname='t_a_key'`, - gosql.NullString{String: `unique_comment`, Valid: true}, - }, - { - `COMMENT ON CONSTRAINT t_a_key ON s.t IS 'unique_comment'`, - `SELECT obj_description(oid, 'pg_constraint') FROM pg_constraint WHERE conname='t_a_key'`, - gosql.NullString{String: `unique_comment`, Valid: true}, - }, - { - `COMMENT ON CONSTRAINT positive_price ON t IS 'check_comment'`, - `SELECT obj_description(oid, 'pg_constraint') FROM pg_constraint WHERE conname='positive_price'`, - gosql.NullString{String: `check_comment`, Valid: true}, - }, - { - `COMMENT ON CONSTRAINT check_b_c ON t IS 'check_defaultname_comment'`, - `SELECT obj_description(oid, 'pg_constraint') FROM pg_constraint WHERE conname='check_b_c'`, - gosql.NullString{String: `check_defaultname_comment`, Valid: true}, - }, - { - `COMMENT ON CONSTRAINT pkey ON t IS 'primary_userdef_comment'`, - `SELECT obj_description(oid, 'pg_constraint') FROM pg_constraint WHERE conname='pkey'`, - gosql.NullString{String: `primary_userdef_comment`, Valid: true}, - }, - { - `COMMENT ON CONSTRAINT "t2_pkey" ON t2 IS 'primary_comment'`, - `SELECT obj_description(oid, 'pg_constraint') FROM pg_constraint WHERE conname='t2_pkey'`, - gosql.NullString{String: `primary_comment`, Valid: true}, - }, - { - `COMMENT ON CONSTRAINT t2_b_fkey ON t2 IS 'fk_comment'`, - `SELECT obj_description(oid, 'pg_constraint') FROM pg_constraint WHERE conname='t2_b_fkey'`, - gosql.NullString{String: `fk_comment`, Valid: true}, - }, - { - `COMMENT ON CONSTRAINT t2_b_fkey ON t2 IS 'fk_comment'; COMMENT ON CONSTRAINT t2_b_fkey ON t2 IS NULL`, - `SELECT obj_description(oid, 'pg_constraint') FROM pg_constraint WHERE conname='t2_b_fkey'`, - gosql.NullString{Valid: false}, - }, - } + testCases := []struct { + exec string + query string + expect gosql.NullString + }{ + { + `COMMENT ON CONSTRAINT t_a_key ON t IS 'unique_comment'`, + `SELECT obj_description(oid, 'pg_constraint') FROM pg_constraint WHERE conname='t_a_key'`, + gosql.NullString{String: `unique_comment`, Valid: true}, + }, + { + `COMMENT ON CONSTRAINT t_a_key ON s.t IS 'unique_comment'`, + `SELECT obj_description(oid, 'pg_constraint') FROM pg_constraint WHERE conname='t_a_key'`, + gosql.NullString{String: `unique_comment`, Valid: true}, + }, + { + `COMMENT ON CONSTRAINT positive_price ON t IS 'check_comment'`, + `SELECT obj_description(oid, 'pg_constraint') FROM pg_constraint WHERE conname='positive_price'`, + gosql.NullString{String: `check_comment`, Valid: true}, + }, + { + `COMMENT ON CONSTRAINT check_b_c ON t IS 'check_defaultname_comment'`, + `SELECT obj_description(oid, 'pg_constraint') FROM pg_constraint WHERE conname='check_b_c'`, + gosql.NullString{String: `check_defaultname_comment`, Valid: true}, + }, + { + `COMMENT ON CONSTRAINT pkey ON t IS 'primary_userdef_comment'`, + `SELECT obj_description(oid, 'pg_constraint') FROM pg_constraint WHERE conname='pkey'`, + gosql.NullString{String: `primary_userdef_comment`, Valid: true}, + }, + { + `COMMENT ON CONSTRAINT "t2_pkey" ON t2 IS 'primary_comment'`, + `SELECT obj_description(oid, 'pg_constraint') FROM pg_constraint WHERE conname='t2_pkey'`, + gosql.NullString{String: `primary_comment`, Valid: true}, + }, + { + `COMMENT ON CONSTRAINT t2_b_fkey ON t2 IS 'fk_comment'`, + `SELECT obj_description(oid, 'pg_constraint') FROM pg_constraint WHERE conname='t2_b_fkey'`, + gosql.NullString{String: `fk_comment`, Valid: true}, + }, + { + `COMMENT ON CONSTRAINT t2_b_fkey ON t2 IS 'fk_comment'; COMMENT ON CONSTRAINT t2_b_fkey ON t2 IS NULL`, + `SELECT obj_description(oid, 'pg_constraint') FROM pg_constraint WHERE conname='t2_b_fkey'`, + gosql.NullString{Valid: false}, + }, + } - for _, tc := range testCases { - t.Run(tc.exec, func(t *testing.T) { - if _, err := db.Exec(tc.exec); err != nil { - t.Fatal(err) - } + for _, tc := range testCases { + t.Run(tc.exec, func(t *testing.T) { + if _, err := db.Exec(tc.exec); err != nil { + t.Fatal(err) + } - row := db.QueryRow(tc.query) - var comment gosql.NullString - if err := row.Scan(&comment); err != nil { - t.Fatal(err) - } - if tc.expect != comment { - t.Fatalf("expected comment %v, got %v", tc.expect, comment) - } - }) - } + row := db.QueryRow(tc.query) + var comment gosql.NullString + if err := row.Scan(&comment); err != nil { + t.Fatal(err) + } + if tc.expect != comment { + t.Fatalf("expected comment %v, got %v", tc.expect, comment) + } + }) + } + }) } diff --git a/pkg/sql/comment_on_database_test.go b/pkg/sql/comment_on_database_test.go index 7f690d5b1d41..5904a8ab0dfa 100644 --- a/pkg/sql/comment_on_database_test.go +++ b/pkg/sql/comment_on_database_test.go @@ -11,12 +11,9 @@ package sql_test import ( - "context" gosql "database/sql" "testing" - "github.com/cockroachdb/cockroach/pkg/sql/tests" - "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/errors" @@ -26,84 +23,80 @@ func TestCommentOnDatabase(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - params, _ := tests.CreateTestServerParams() - s, db, _ := serverutils.StartServer(t, params) - defer s.Stopper().Stop(context.Background()) - - if _, err := db.Exec(` + runCommentOnTests(t, func(db *gosql.DB) { + if _, err := db.Exec(` CREATE DATABASE d; `); err != nil { - t.Fatal(err) - } - - testCases := []struct { - exec string - query string - expect gosql.NullString - }{ - { - `COMMENT ON DATABASE d IS 'foo'`, - `SELECT shobj_description(oid, 'pg_database') FROM pg_database WHERE datname = 'd'`, - gosql.NullString{String: `foo`, Valid: true}, - }, - { - `ALTER DATABASE d RENAME TO d2`, - `SELECT shobj_description(oid, 'pg_database') FROM pg_database WHERE datname = 'd2'`, - gosql.NullString{String: `foo`, Valid: true}, - }, - { - `COMMENT ON DATABASE d2 IS NULL`, - `SELECT shobj_description(oid, 'pg_database') FROM pg_database WHERE datname = 'd2'`, - gosql.NullString{Valid: false}, - }, - } - - for _, tc := range testCases { - if _, err := db.Exec(tc.exec); err != nil { t.Fatal(err) } - row := db.QueryRow(tc.query) - var comment gosql.NullString - if err := row.Scan(&comment); err != nil { - t.Fatal(err) + testCases := []struct { + exec string + query string + expect gosql.NullString + }{ + { + `COMMENT ON DATABASE d IS 'foo'`, + `SELECT shobj_description(oid, 'pg_database') FROM pg_database WHERE datname = 'd'`, + gosql.NullString{String: `foo`, Valid: true}, + }, + { + `ALTER DATABASE d RENAME TO d2`, + `SELECT shobj_description(oid, 'pg_database') FROM pg_database WHERE datname = 'd2'`, + gosql.NullString{String: `foo`, Valid: true}, + }, + { + `COMMENT ON DATABASE d2 IS NULL`, + `SELECT shobj_description(oid, 'pg_database') FROM pg_database WHERE datname = 'd2'`, + gosql.NullString{Valid: false}, + }, } - if tc.expect != comment { - t.Fatalf("expected comment %v, got %v", tc.expect, comment) + + for _, tc := range testCases { + if _, err := db.Exec(tc.exec); err != nil { + t.Fatal(err) + } + + row := db.QueryRow(tc.query) + var comment gosql.NullString + if err := row.Scan(&comment); err != nil { + t.Fatal(err) + } + if tc.expect != comment { + t.Fatalf("expected comment %v, got %v", tc.expect, comment) + } } - } + }) } func TestCommentOnDatabaseWhenDrop(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - params, _ := tests.CreateTestServerParams() - s, db, _ := serverutils.StartServer(t, params) - defer s.Stopper().Stop(context.Background()) - - if _, err := db.Exec(` + runCommentOnTests(t, func(db *gosql.DB) { + if _, err := db.Exec(` CREATE DATABASE d; `); err != nil { - t.Fatal(err) - } - - if _, err := db.Exec(`COMMENT ON DATABASE d IS 'foo'`); err != nil { - t.Fatal(err) - } + t.Fatal(err) + } - if _, err := db.Exec(`DROP DATABASE d`); err != nil { - t.Fatal(err) - } + if _, err := db.Exec(`COMMENT ON DATABASE d IS 'foo'`); err != nil { + t.Fatal(err) + } - row := db.QueryRow(`SELECT comment FROM system.comments LIMIT 1`) - var comment string - err := row.Scan(&comment) - if !errors.Is(err, gosql.ErrNoRows) { - if err != nil { + if _, err := db.Exec(`DROP DATABASE d`); err != nil { t.Fatal(err) } - t.Fatal("comment remaining in system.comments despite drop") - } + row := db.QueryRow(`SELECT comment FROM system.comments LIMIT 1`) + var comment string + err := row.Scan(&comment) + if !errors.Is(err, gosql.ErrNoRows) { + if err != nil { + t.Fatal(err) + } + + t.Fatal("comment remaining in system.comments despite drop") + } + }) } diff --git a/pkg/sql/comment_on_index_test.go b/pkg/sql/comment_on_index_test.go index 2f8849492773..09679f650280 100644 --- a/pkg/sql/comment_on_index_test.go +++ b/pkg/sql/comment_on_index_test.go @@ -11,12 +11,9 @@ package sql_test import ( - "context" gosql "database/sql" "testing" - "github.com/cockroachdb/cockroach/pkg/sql/tests" - "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/errors" @@ -26,124 +23,118 @@ func TestCommentOnIndex(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - params, _ := tests.CreateTestServerParams() - s, db, _ := serverutils.StartServer(t, params) - defer s.Stopper().Stop(context.Background()) - - if _, err := db.Exec(` + runCommentOnTests(t, func(db *gosql.DB) { + if _, err := db.Exec(` CREATE DATABASE d; SET DATABASE = d; CREATE TABLE t (c INT, INDEX t_c_idx (c)); `); err != nil { - t.Fatal(err) - } - - testCases := []struct { - exec string - query string - expect gosql.NullString - }{ - { - `COMMENT ON INDEX t_c_idx IS 'index_comment'`, - `SELECT obj_description(oid) from pg_class WHERE relname='t_c_idx';`, - gosql.NullString{String: `index_comment`, Valid: true}, - }, - { - `TRUNCATE t`, - `SELECT obj_description(oid) from pg_class WHERE relname='t_c_idx';`, - gosql.NullString{String: `index_comment`, Valid: true}, - }, - { - `COMMENT ON INDEX t_c_idx IS NULL`, - `SELECT obj_description(oid) from pg_class WHERE relname='t_c_idx';`, - gosql.NullString{Valid: false}, - }, - } - - for _, tc := range testCases { - if _, err := db.Exec(tc.exec); err != nil { t.Fatal(err) } - row := db.QueryRow(tc.query) - var comment gosql.NullString - if err := row.Scan(&comment); err != nil { - t.Fatal(err) + testCases := []struct { + exec string + query string + expect gosql.NullString + }{ + { + `COMMENT ON INDEX t_c_idx IS 'index_comment'`, + `SELECT obj_description(oid) from pg_class WHERE relname='t_c_idx';`, + gosql.NullString{String: `index_comment`, Valid: true}, + }, + { + `TRUNCATE t`, + `SELECT obj_description(oid) from pg_class WHERE relname='t_c_idx';`, + gosql.NullString{String: `index_comment`, Valid: true}, + }, + { + `COMMENT ON INDEX t_c_idx IS NULL`, + `SELECT obj_description(oid) from pg_class WHERE relname='t_c_idx';`, + gosql.NullString{Valid: false}, + }, } - if tc.expect != comment { - t.Fatalf("expected comment %v, got %v", tc.expect, comment) + + for _, tc := range testCases { + if _, err := db.Exec(tc.exec); err != nil { + t.Fatal(err) + } + + row := db.QueryRow(tc.query) + var comment gosql.NullString + if err := row.Scan(&comment); err != nil { + t.Fatal(err) + } + if tc.expect != comment { + t.Fatalf("expected comment %v, got %v", tc.expect, comment) + } } - } + }) } func TestCommentOnIndexWhenDropTable(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - params, _ := tests.CreateTestServerParams() - s, db, _ := serverutils.StartServer(t, params) - defer s.Stopper().Stop(context.Background()) - - if _, err := db.Exec(` + runCommentOnTests(t, func(db *gosql.DB) { + if _, err := db.Exec(` CREATE DATABASE d; SET DATABASE = d; CREATE TABLE t (c INT, INDEX t_c_idx (c)); `); err != nil { - t.Fatal(err) - } - - if _, err := db.Exec(`COMMENT ON INDEX t_c_idx IS 'index_comment'`); err != nil { - t.Fatal(err) - } - - if _, err := db.Exec(`DROP TABLE t`); err != nil { - t.Fatal(err) - } - - row := db.QueryRow(`SELECT comment FROM system.comments LIMIT 1`) - var comment string - err := row.Scan(&comment) - if !errors.Is(err, gosql.ErrNoRows) { - if err != nil { t.Fatal(err) } - t.Fatal("comment remain") - } + if _, err := db.Exec(`COMMENT ON INDEX t_c_idx IS 'index_comment'`); err != nil { + t.Fatal(err) + } + + if _, err := db.Exec(`DROP TABLE t`); err != nil { + t.Fatal(err) + } + + row := db.QueryRow(`SELECT comment FROM system.comments LIMIT 1`) + var comment string + err := row.Scan(&comment) + if !errors.Is(err, gosql.ErrNoRows) { + if err != nil { + t.Fatal(err) + } + + t.Fatal("comment remain") + } + }) } func TestCommentOnIndexWhenDropIndex(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - params, _ := tests.CreateTestServerParams() - s, db, _ := serverutils.StartServer(t, params) - defer s.Stopper().Stop(context.Background()) - - if _, err := db.Exec(` + runCommentOnTests(t, func(db *gosql.DB) { + if _, err := db.Exec(` CREATE DATABASE d; SET DATABASE = d; CREATE TABLE t (c INT, INDEX t_c_idx (c)); `); err != nil { - t.Fatal(err) - } - - if _, err := db.Exec(`COMMENT ON INDEX t_c_idx IS 'index_comment'`); err != nil { - t.Fatal(err) - } - - if _, err := db.Exec(`DROP INDEX t_c_idx`); err != nil { - t.Fatal(err) - } - - row := db.QueryRow(`SELECT comment FROM system.comments LIMIT 1`) - var comment string - err := row.Scan(&comment) - if !errors.Is(err, gosql.ErrNoRows) { - if err != nil { t.Fatal(err) } - t.Fatal("comment remain") - } + if _, err := db.Exec(`COMMENT ON INDEX t_c_idx IS 'index_comment'`); err != nil { + t.Fatal(err) + } + + if _, err := db.Exec(`DROP INDEX t_c_idx`); err != nil { + t.Fatal(err) + } + + row := db.QueryRow(`SELECT comment FROM system.comments LIMIT 1`) + var comment string + err := row.Scan(&comment) + if !errors.Is(err, gosql.ErrNoRows) { + if err != nil { + t.Fatal(err) + } + + t.Fatal("comment remain") + } + }) } diff --git a/pkg/sql/comment_on_schema_test.go b/pkg/sql/comment_on_schema_test.go index d376789d1ebd..f77746bd30aa 100644 --- a/pkg/sql/comment_on_schema_test.go +++ b/pkg/sql/comment_on_schema_test.go @@ -11,12 +11,9 @@ package sql_test import ( - "context" gosql "database/sql" "testing" - "github.com/cockroachdb/cockroach/pkg/sql/tests" - "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/errors" @@ -26,84 +23,80 @@ func TestCommentOnSchema(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - params, _ := tests.CreateTestServerParams() - s, db, _ := serverutils.StartServer(t, params) - defer s.Stopper().Stop(context.Background()) - - if _, err := db.Exec(` + runCommentOnTests(t, func(db *gosql.DB) { + if _, err := db.Exec(` CREATE SCHEMA d; `); err != nil { - t.Fatal(err) - } - - testCases := []struct { - exec string - query string - expect gosql.NullString - }{ - { - `COMMENT ON SCHEMA d IS 'foo'`, - `SELECT obj_description(oid, 'pg_namespace') FROM pg_namespace WHERE nspname = 'd'`, - gosql.NullString{String: `foo`, Valid: true}, - }, - { - `ALTER SCHEMA d RENAME TO d2`, - `SELECT obj_description(oid, 'pg_namespace') FROM pg_namespace WHERE nspname = 'd2'`, - gosql.NullString{String: `foo`, Valid: true}, - }, - { - `COMMENT ON SCHEMA d2 IS NULL`, - `SELECT obj_description(oid, 'pg_namespace') FROM pg_namespace WHERE nspname = 'd2'`, - gosql.NullString{Valid: false}, - }, - } - - for _, tc := range testCases { - if _, err := db.Exec(tc.exec); err != nil { t.Fatal(err) } - row := db.QueryRow(tc.query) - var comment gosql.NullString - if err := row.Scan(&comment); err != nil { - t.Fatal(err) + testCases := []struct { + exec string + query string + expect gosql.NullString + }{ + { + `COMMENT ON SCHEMA d IS 'foo'`, + `SELECT obj_description(oid, 'pg_namespace') FROM pg_namespace WHERE nspname = 'd'`, + gosql.NullString{String: `foo`, Valid: true}, + }, + { + `ALTER SCHEMA d RENAME TO d2`, + `SELECT obj_description(oid, 'pg_namespace') FROM pg_namespace WHERE nspname = 'd2'`, + gosql.NullString{String: `foo`, Valid: true}, + }, + { + `COMMENT ON SCHEMA d2 IS NULL`, + `SELECT obj_description(oid, 'pg_namespace') FROM pg_namespace WHERE nspname = 'd2'`, + gosql.NullString{Valid: false}, + }, } - if tc.expect != comment { - t.Fatalf("expected comment %v, got %v", tc.expect, comment) + + for _, tc := range testCases { + if _, err := db.Exec(tc.exec); err != nil { + t.Fatal(err) + } + + row := db.QueryRow(tc.query) + var comment gosql.NullString + if err := row.Scan(&comment); err != nil { + t.Fatal(err) + } + if tc.expect != comment { + t.Fatalf("expected comment %v, got %v", tc.expect, comment) + } } - } + }) } func TestCommentOnSchemaWhenDrop(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - params, _ := tests.CreateTestServerParams() - s, db, _ := serverutils.StartServer(t, params) - defer s.Stopper().Stop(context.Background()) - - if _, err := db.Exec(` + runCommentOnTests(t, func(db *gosql.DB) { + if _, err := db.Exec(` CREATE SCHEMA d; `); err != nil { - t.Fatal(err) - } - - if _, err := db.Exec(`COMMENT ON SCHEMA d IS 'foo'`); err != nil { - t.Fatal(err) - } + t.Fatal(err) + } - if _, err := db.Exec(`DROP SCHEMA d`); err != nil { - t.Fatal(err) - } + if _, err := db.Exec(`COMMENT ON SCHEMA d IS 'foo'`); err != nil { + t.Fatal(err) + } - row := db.QueryRow(`SELECT comment FROM system.comments LIMIT 1`) - var comment string - err := row.Scan(&comment) - if !errors.Is(err, gosql.ErrNoRows) { - if err != nil { + if _, err := db.Exec(`DROP SCHEMA d`); err != nil { t.Fatal(err) } - t.Fatal("comment remaining in system.comments despite drop") - } + row := db.QueryRow(`SELECT comment FROM system.comments LIMIT 1`) + var comment string + err := row.Scan(&comment) + if !errors.Is(err, gosql.ErrNoRows) { + if err != nil { + t.Fatal(err) + } + + t.Fatal("comment remaining in system.comments despite drop") + } + }) } diff --git a/pkg/sql/comment_on_table_test.go b/pkg/sql/comment_on_table_test.go index 6e380852e904..e9c60ec5a8cc 100644 --- a/pkg/sql/comment_on_table_test.go +++ b/pkg/sql/comment_on_table_test.go @@ -11,12 +11,9 @@ package sql_test import ( - "context" gosql "database/sql" "testing" - "github.com/cockroachdb/cockroach/pkg/sql/tests" - "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/errors" @@ -26,88 +23,84 @@ func TestCommentOnTable(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - params, _ := tests.CreateTestServerParams() - s, db, _ := serverutils.StartServer(t, params) - defer s.Stopper().Stop(context.Background()) - - if _, err := db.Exec(` + runCommentOnTests(t, func(db *gosql.DB) { + if _, err := db.Exec(` CREATE DATABASE d; SET DATABASE = d; CREATE TABLE t (i INT ); `); err != nil { - t.Fatal(err) - } - - testCases := []struct { - exec string - query string - expect gosql.NullString - }{ - { - `COMMENT ON TABLE t IS 'foo'`, - `SELECT obj_description('t'::regclass)`, - gosql.NullString{String: `foo`, Valid: true}, - }, - { - `TRUNCATE t`, - `SELECT obj_description('t'::regclass)`, - gosql.NullString{String: `foo`, Valid: true}, - }, - { - `COMMENT ON TABLE t IS NULL`, - `SELECT obj_description('t'::regclass)`, - gosql.NullString{Valid: false}, - }, - } - - for _, tc := range testCases { - if _, err := db.Exec(tc.exec); err != nil { t.Fatal(err) } - row := db.QueryRow(tc.query) - var comment gosql.NullString - if err := row.Scan(&comment); err != nil { - t.Fatal(err) + testCases := []struct { + exec string + query string + expect gosql.NullString + }{ + { + `COMMENT ON TABLE t IS 'foo'`, + `SELECT obj_description('t'::regclass)`, + gosql.NullString{String: `foo`, Valid: true}, + }, + { + `TRUNCATE t`, + `SELECT obj_description('t'::regclass)`, + gosql.NullString{String: `foo`, Valid: true}, + }, + { + `COMMENT ON TABLE t IS NULL`, + `SELECT obj_description('t'::regclass)`, + gosql.NullString{Valid: false}, + }, } - if tc.expect != comment { - t.Fatalf("expected comment %v, got %v", tc.expect, comment) + + for _, tc := range testCases { + if _, err := db.Exec(tc.exec); err != nil { + t.Fatal(err) + } + + row := db.QueryRow(tc.query) + var comment gosql.NullString + if err := row.Scan(&comment); err != nil { + t.Fatal(err) + } + if tc.expect != comment { + t.Fatalf("expected comment %v, got %v", tc.expect, comment) + } } - } + }) } func TestCommentOnTableWhenDrop(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - params, _ := tests.CreateTestServerParams() - s, db, _ := serverutils.StartServer(t, params) - defer s.Stopper().Stop(context.Background()) - - if _, err := db.Exec(` + runCommentOnTests(t, func(db *gosql.DB) { + if _, err := db.Exec(` CREATE DATABASE d; SET DATABASE = d; CREATE TABLE t (i INT ); `); err != nil { - t.Fatal(err) - } - - if _, err := db.Exec(`COMMENT ON TABLE t IS 'foo'`); err != nil { - t.Fatal(err) - } + t.Fatal(err) + } - if _, err := db.Exec(`DROP TABLE t`); err != nil { - t.Fatal(err) - } + if _, err := db.Exec(`COMMENT ON TABLE t IS 'foo'`); err != nil { + t.Fatal(err) + } - row := db.QueryRow(`SELECT comment FROM system.comments LIMIT 1`) - var comment string - err := row.Scan(&comment) - if !errors.Is(err, gosql.ErrNoRows) { - if err != nil { + if _, err := db.Exec(`DROP TABLE t`); err != nil { t.Fatal(err) } - t.Fatal("comment remaining in system.comments despite drop") - } + row := db.QueryRow(`SELECT comment FROM system.comments LIMIT 1`) + var comment string + err := row.Scan(&comment) + if !errors.Is(err, gosql.ErrNoRows) { + if err != nil { + t.Fatal(err) + } + + t.Fatal("comment remaining in system.comments despite drop") + } + }) } diff --git a/pkg/sql/drop_database.go b/pkg/sql/drop_database.go index e13ba1cecdf1..e04084f1cb91 100644 --- a/pkg/sql/drop_database.go +++ b/pkg/sql/drop_database.go @@ -13,6 +13,7 @@ package sql import ( "context" + "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" @@ -203,6 +204,14 @@ func (n *dropDatabaseNode) startExec(params runParams) error { return err } + if err := metadataUpdater.DeleteDescriptorComment( + int64(n.dbDesc.GetID()), + 0, /* subID */ + keys.DatabaseCommentType, + ); err != nil { + return err + } + // Log Drop Database event. This is an auditable log event and is recorded // in the same transaction as the table descriptor update. return p.logEvent(ctx,