Skip to content

Commit

Permalink
sql: drop comment when drop database in legacy schema changer.
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@chengxiong-ruan
chengxiong-ruan committed Nov 10, 2022
1 parent 1b308ee commit d4620be
Show file tree
Hide file tree
Showing 7 changed files with 457 additions and 475 deletions.
262 changes: 135 additions & 127 deletions pkg/sql/comment_on_column_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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)
}()
}
}
Loading

0 comments on commit d4620be

Please sign in to comment.