pg/catalog: pin four migration behaviors bytebase was testing for us - #407
Merged
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e177d698dd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
d-bytebase
force-pushed
the
move-pg-sdl-migration-tests
branch
from
September 3, 2026 17:11
e177d69 to
6d6ba1b
Compare
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 <noreply@anthropic.com>
d-bytebase
force-pushed
the
move-pg-sdl-migration-tests
branch
from
September 3, 2026 17:12
6d6ba1b to
0a2c14b
Compare
d-bytebase
changed the title
d-bytebase
added a commit
to bytebase/bytebase
that referenced
this pull request
Sep 3, 2026
Effort 7 of docs/design/backend-test-execution-time.md says engine conformance belongs in omni. 24 files in backend/plugin/schema/pg were already omni's in substance: they call omni/pg/catalog's LoadSDL, Diff and GenerateMigration directly and name no bytebase symbol at all. Migration-era leftovers from when we swapped the ANTLR SDL engine for omni's. They do not move as they are. Read against omni's own pg/catalog suite, all but four of their behaviors are covered there already, by more cases and with better assertions — they match substrings of the rendered SQL where omni inspects typed migration ops. Two of them cannot fail at all: neither test named for EXCLUDE constraints has an EXCLUDE constraint in its fixture, and one asserts the absence of an index name that appears nowhere. The four genuine gaps went to omni beside the cases they belong with, in that suite's shape — bytebase/omni#407. The rest are deleted here. The package goes from 15.1 s to 12.0 s. Looking for the rest of effort 7 found that the metadata proto is not the near blocker it is written up as. omni has no Oracle or MSSQL engine to move to — only a parser and AST, which those packages already use for their own extractors — and schema/tidb uses none of the TiDB catalog omni does have. Every container test in plugin/schema/* calls a bytebase-owned entry point, so the 272 s of Oracle and 51 s of MSSQL stay until the implementations move. Section 7 now records that per engine. omniSDLMigration and diffPostgresSDL stay: omni_filter_test.go and sdl_migration_omni_test.go still use them. go vet, golangci-lint and go test ./backend/plugin/schema/... all pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a2c14b5c3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
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 <noreply@anthropic.com>
rebelice
approved these changes
Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed since the first push
This opened as a straight move of 24 test files out of bytebase's
backend/plugin/schema/pg. Reading them against whatpg/catalogalready tests, that was the wrong shape, so the PR is now +106 lines across 3 existing files instead of +6,363 across 25 new ones.Why
The 24 files called this package directly —
LoadSDL,Diff,GenerateMigration— with no bytebase symbol in them, so they did belong here. Butpg/catalogalready tests nearly all of what they cover, with more cases and better assertions:column_sdl_diff,column_migration_integrationdiff_column_test.go(15 cases incl. identity, generated, collation),migration_column_test.gocomment_migration,get_sdl_diff_comment,procedure_sdl_diffdiff_comment_test.go(13 cases),migration_comment_test.go(incl. "COMMENT ON VIEW uses VIEW not TABLE", "COMMENT ON PROCEDURE uses PROCEDURE not FUNCTION")sequence_sdl_diff,sequence_migration_integrationdiff_sequence_test.go(incl.TestDiffSequenceSerialAbsorption),migration_sequence_test.goview_sdl_diff,materialized_view_sdl_diff,view_migration_integrationdiff_view_test.go,migration_view_test.go(incl. "Matview indexes generated after matview creation")standalone_index,camelcase_indexmigration_index_test.go, andTestContainerIdentifierQuoting/camelCase_in_index— which proves it against a real Postgrestrigger_sdl_diff,trigger_drop_dependencydiff_trigger_test.go(12 cases),migration_ordering_test.go"2.2 drop table + dependent trigger → trigger dropped before table"schema_implicit_creationmigration_schema_test.go"schema operations ordered before table operations"diff_audit(10 behaviors)migration_table_test.go"DROP TABLE CASCADE",migration_comment_test.goview/matview/procedure targets,diff_sequence_test.goabsorption,migration_trigger_test.go"modified trigger as DROP plus CREATE"parsing_migrationextension_test.go,migration_extension_test.go,diff_column_test.goidentity casesThey also assert differently from everything here:
require.Contains(plan.SQL(), "…")against one concatenated blob, wherepg/cataloginspects typed ops. That is what produced all four Codex findings on the first push, and two of the files were worse than duplicative —TestExcludeConstraintIndexNotDuplicatedasserts the absence of an index name that appears nowhere in its fixture, which no code change can ever fail, and neither "exclude constraint" test has an EXCLUDE constraint in it at all.What is actually new
Four behaviors were not covered anywhere here. Each goes into the file it belongs to, in that file's shape:
migration_index_test.go) — sits beside the PK/UNIQUE case that was already pinned. EXCLUDE carries an index the same way; aCREATE INDEXemitted next to theADD CONSTRAINTfails on apply because the constraint already took the name.migration_extension_test.go) — aCREATE SCHEMA "pg_trgm"besideCREATE EXTENSION "pg_trgm"fails anywhere the extension is already installed.migration_comment_test.go) —quoteLiteralhas done this since it was written and nothing tested it. This is Codex's 4th finding, kept rather than dropped.migration_comment_test.go) — procedures and functions share one catalog, soroutine_x()the function androutine_x(integer)the procedure are separated only by their argument lists. A comment differ keyed on the bare name reports the dropped procedure's comment as a change to the surviving function.Bug found on the way
Dropping any commented object emits
COMMENT ON … IS NULLinPhaseMain, after the object's ownDROPinPhasePre, and PostgreSQL has noCOMMENT … IF EXISTS— so the migration fails on apply. Reproduced againstpostgres:16-alpinefor tables, views and procedures alike. Filed as #408; the fix belongs ingenerateCommentDDLand wants a container test of its own, so it is not carried here.Testing
Each new case carries a positive precondition so its negative assertion cannot pass vacuously, and each was checked by mutation — invert the expectation, confirm the test fails:
go build ./...go test -count=1 ./pg/catalog/— pass, 17.6 sgo test -short -count=1 ./pg/catalog/— pass, 4.9 sgo test -tags=oracle -run '^$' -count=1 ./pg/catalog/— compilesThe companion bytebase PR still deletes the 24 files there: bytebase/bytebase#21337.
🤖 Generated with Claude Code