From 0a2c14b5c3b5135881fdeacc558e47153588d7de Mon Sep 17 00:00:00 2001 From: Danny Xu Date: Thu, 3 Sep 2026 17:10:30 +0000 Subject: [PATCH 1/2] pg/catalog: pin four migration behaviors bytebase was testing for us MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bytebase's backend/plugin/schema/pg held 24 test files that called this package directly — LoadSDL, Diff, GenerateMigration — with no bytebase symbol in them. Read against what pg/catalog already tests, all but four of their behaviors are covered here, usually by more cases and always with better assertions: they string-matched plan.SQL() where these files inspect typed ops. So this takes the four that were not covered, in the shape of the file each belongs to, rather than the 6,300 lines. - EXCLUDE backing index NOT generated (migration_index_test.go). The PK/UNIQUE case beside it was already pinned; EXCLUDE carries an index the same way and was not. A CREATE INDEX emitted next to the ADD CONSTRAINT fails on apply. - Extension name does not become a schema (migration_extension_test.go). A CREATE SCHEMA "pg_trgm" beside CREATE EXTENSION "pg_trgm" fails anywhere the extension is already installed. - Single quotes in a comment are doubled (migration_comment_test.go). quoteLiteral has been doubling them since it was written and nothing tested it; a bare quote closes the literal early and the statement stops parsing. - Dropping a procedure leaves an unrelated function's comment alone (migration_comment_test.go). Procedures and functions share one catalog, so a comment differ keying on the bare name reports the dropped procedure's comment as a change to a surviving function. Each carries a positive precondition so the negative assertion cannot pass vacuously, and each was checked by mutation: invert the expectation and the test fails. go build ./..., go test ./pg/catalog/, go test -short ./pg/catalog/ and the -tags=oracle compile all pass. Co-Authored-By: Claude Opus 5 --- pg/catalog/migration_comment_test.go | 64 ++++++++++++++++++++++++++ pg/catalog/migration_extension_test.go | 21 +++++++++ pg/catalog/migration_index_test.go | 21 +++++++++ 3 files changed, 106 insertions(+) diff --git a/pg/catalog/migration_comment_test.go b/pg/catalog/migration_comment_test.go index 99eeb4ad..4d78f9fc 100644 --- a/pg/catalog/migration_comment_test.go +++ b/pg/catalog/migration_comment_test.go @@ -271,6 +271,70 @@ func TestMigrationComment(t *testing.T) { } }) + t.Run("single quotes in a comment are doubled", func(t *testing.T) { + fromSQL := `CREATE TABLE t (id int);` + toSQL := ` + CREATE TABLE t (id int); + COMMENT ON TABLE t IS 'User''s table'; + ` + from, err := LoadSQL(fromSQL) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(toSQL) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := GenerateMigration(from, to, diff) + ops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpComment + }).Ops + if len(ops) != 1 { + t.Fatalf("expected 1 Comment op, got %d; ops: %v", len(ops), opsSQL(plan)) + } + // The catalog holds the comment unescaped. Rendering it back with a bare + // quote closes the literal early and the statement no longer parses, so + // the whole literal has to be checked, not just the text around it. + if !strings.Contains(ops[0].SQL, `IS 'User''s table'`) { + t.Errorf("expected the quote doubled in the emitted literal, got: %s", ops[0].SQL) + } + }) + + t.Run("dropping a procedure leaves an unrelated function's comment alone", func(t *testing.T) { + const survivor = ` + CREATE FUNCTION audit_log() RETURNS void + LANGUAGE plpgsql AS $$ BEGIN NULL; END; $$; + COMMENT ON FUNCTION audit_log() IS 'Writes the audit log'; + ` + fromSQL := survivor + ` + CREATE PROCEDURE purge_rows() LANGUAGE plpgsql AS $$ BEGIN NULL; END; $$; + COMMENT ON PROCEDURE purge_rows() IS 'Purges old rows'; + ` + toSQL := survivor + from, err := LoadSQL(fromSQL) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(toSQL) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := GenerateMigration(from, to, diff) + // Procedures and functions share one catalog, so a comment differ that + // keys on the bare name rather than the full signature reports the + // dropped procedure's comment as a change to the surviving function. + if ops := filterOps(plan, OpDropFunction); len(ops) != 1 { + t.Fatalf("expected 1 DropFunction op for the procedure, got %d; ops: %v", len(ops), opsSQL(plan)) + } + for _, op := range plan.Ops { + if op.Type == OpComment && strings.Contains(op.SQL, "audit_log") { + t.Errorf("the surviving function's comment must not change: %s", op.SQL) + } + } + }) + t.Run("COMMENT ON PROCEDURE uses PROCEDURE not FUNCTION", func(t *testing.T) { fromSQL := ` CREATE PROCEDURE do_work(x integer) diff --git a/pg/catalog/migration_extension_test.go b/pg/catalog/migration_extension_test.go index e995cb8e..402d8d54 100644 --- a/pg/catalog/migration_extension_test.go +++ b/pg/catalog/migration_extension_test.go @@ -86,6 +86,27 @@ func TestMigrationExtension(t *testing.T) { } }, }, + { + name: "Extension name does not become a schema", + fromSQL: "", + toSQL: "CREATE EXTENSION test_ext; CREATE TABLE t1 (id int);", + check: func(t *testing.T, plan *MigrationPlan) { + // CREATE EXTENSION names an extension, not a schema. Registering + // one under its own name would emit a CREATE SCHEMA that fails on + // any database where the extension is already installed. + extOps := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpCreateExtension + }) + if len(extOps.Ops) != 1 { + t.Fatalf("expected 1 CreateExtension op, got %d", len(extOps.Ops)) + } + for _, op := range plan.Ops { + if op.Type == OpCreateSchema { + t.Errorf("extension must not produce a schema, got: %s", op.SQL) + } + } + }, + }, { name: "Extension operations ordered before types and tables", fromSQL: "", diff --git a/pg/catalog/migration_index_test.go b/pg/catalog/migration_index_test.go index 0ab1b0a9..a16344f0 100644 --- a/pg/catalog/migration_index_test.go +++ b/pg/catalog/migration_index_test.go @@ -191,6 +191,27 @@ CREATE INDEX idx_t_name ON t (name, email);`, } }, }, + { + name: "EXCLUDE backing index NOT generated", + fromSQL: `CREATE TABLE t (id int, r int4range);`, + toSQL: `CREATE TABLE t (id int, r int4range, CONSTRAINT t_r_excl EXCLUDE USING gist (r WITH &&));`, + check: func(t *testing.T, plan *MigrationPlan) { + // EXCLUDE carries its own index the way PK and UNIQUE do, so the + // ADD CONSTRAINT is the whole change. A CREATE INDEX beside it + // would fail on apply: the name is already taken by the + // constraint's index. + if ops := filterOps(plan, OpCreateIndex); len(ops) != 0 { + t.Errorf("expected no CreateIndex op for EXCLUDE backing index, got %d: %s", len(ops), ops[0].SQL) + } + ops := filterOps(plan, OpAddConstraint) + if len(ops) != 1 { + t.Fatalf("expected 1 AddConstraint op, got %d", len(ops)) + } + if !strings.Contains(ops[0].SQL, "EXCLUDE") { + t.Errorf("expected EXCLUDE in constraint DDL, got %s", ops[0].SQL) + } + }, + }, { name: "PK/UNIQUE backing indexes NOT generated", fromSQL: `CREATE TABLE t (id int);`, From 1371c6dcf4bbd1f3a01149a11eadca8b2645a9b7 Mon Sep 17 00:00:00 2001 From: Danny Xu Date: Fri, 4 Sep 2026 01:15:18 +0000 Subject: [PATCH 2/2] pg/catalog: make the routine-comment case collide on name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The function and the procedure had different names, so a comment differ keyed on the bare name rather than the full signature would still have kept them apart and the test would still have passed. They now share the name routine_x and differ only in arguments, which is the shape that gets it wrong, and the assertion matches on routine_x() so the survivor is distinguishable from the routine_x(integer) being dropped. Checked by mutation: relax the match to the bare name and the case fails on `COMMENT ON PROCEDURE public.routine_x(integer) IS NULL`. That op is itself invalid — it runs in PhaseMain, after the procedure's own DROP in PhasePre, and PostgreSQL has no COMMENT ... IF EXISTS. It is not specific to procedures; dropping any commented table or view produces the same unapplyable plan. Filed as #408 rather than fixed here, since the fix belongs in generateCommentDDL and wants a container test of its own. Co-Authored-By: Claude Opus 5 --- pg/catalog/migration_comment_test.go | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/pg/catalog/migration_comment_test.go b/pg/catalog/migration_comment_test.go index 4d78f9fc..159747cc 100644 --- a/pg/catalog/migration_comment_test.go +++ b/pg/catalog/migration_comment_test.go @@ -301,15 +301,19 @@ func TestMigrationComment(t *testing.T) { } }) - t.Run("dropping a procedure leaves an unrelated function's comment alone", func(t *testing.T) { + t.Run("dropping a procedure leaves a same-named function's comment alone", func(t *testing.T) { + // Procedures and functions share one catalog, and these two share a name + // as well — only the argument list separates them. That is the shape a + // comment differ keyed on the bare name gets wrong: it reads the dropped + // procedure's comment as a change to the surviving function. const survivor = ` - CREATE FUNCTION audit_log() RETURNS void + CREATE FUNCTION routine_x() RETURNS void LANGUAGE plpgsql AS $$ BEGIN NULL; END; $$; - COMMENT ON FUNCTION audit_log() IS 'Writes the audit log'; + COMMENT ON FUNCTION routine_x() IS 'The function'; ` fromSQL := survivor + ` - CREATE PROCEDURE purge_rows() LANGUAGE plpgsql AS $$ BEGIN NULL; END; $$; - COMMENT ON PROCEDURE purge_rows() IS 'Purges old rows'; + CREATE PROCEDURE routine_x(n integer) LANGUAGE plpgsql AS $$ BEGIN NULL; END; $$; + COMMENT ON PROCEDURE routine_x(integer) IS 'The procedure'; ` toSQL := survivor from, err := LoadSQL(fromSQL) @@ -322,14 +326,15 @@ func TestMigrationComment(t *testing.T) { } diff := Diff(from, to) plan := GenerateMigration(from, to, diff) - // Procedures and functions share one catalog, so a comment differ that - // keys on the bare name rather than the full signature reports the - // dropped procedure's comment as a change to the surviving function. if ops := filterOps(plan, OpDropFunction); len(ops) != 1 { t.Fatalf("expected 1 DropFunction op for the procedure, got %d; ops: %v", len(ops), opsSQL(plan)) } + // routine_x() is the survivor, routine_x(integer) the one being dropped. + // The plan still clears the dropped routine's comment, which is issue + // #408 and not what this case is about; matching on the full signature + // keeps the two apart. for _, op := range plan.Ops { - if op.Type == OpComment && strings.Contains(op.SQL, "audit_log") { + if op.Type == OpComment && strings.Contains(op.SQL, "routine_x()") { t.Errorf("the surviving function's comment must not change: %s", op.SQL) } }