feat(pg/catalog): add DropsFromDiff for fast destructive-change detection#21
Draft
feat(pg/catalog): add DropsFromDiff for fast destructive-change detection#21
Conversation
DropsFromDiff returns a MigrationPlan containing only the destructive operations that GenerateMigration would produce for the same inputs, without building SQL text or running the dependency-driven topological sort. Callers that only need to know *what* will be dropped (e.g., destructive-change advisories) can skip GenerateMigration's cost — on large schemas this can save tens of seconds and over a gigabyte of transient heap. All metadata fields (Type, SchemaName, ObjectName, ParentObject, Phase, ObjType, ObjOID, Priority) are populated identically to what GenerateMigration sets; only SQL is left empty (documented contract). Operations are sorted lexicographically by (SchemaName, ObjectName, ParentObject, Type) for determinism instead of topologically. This commit adds the helper skeleton plus 8 of ~17 producer arms, each mirroring the DiffDrop (and where applicable DiffModify) arm of its corresponding generate*DDL function field-for-field: - dropsForSchemas (migration_schema.go) - dropsForEnums (migration_enum.go) - dropsForDomains (migration_domain.go) - dropsForRanges (migration_range.go) - dropsForComposites (migration_composite.go) [ObjType='r', not 't'] - dropsForSequences (migration_sequence.go) [isIdentitySequence filter] - dropsForFunctions (migration_function.go) [entry.Identity, signatureChanged] - dropsForTables (migration_table.go) [RelKind 'r'/'p' only] Remaining producers (table recreate, columns + cascaded checks, constraints, triggers, views/matviews, dependent-view cascade) plus the differential test against GenerateMigration will land in follow-up commits. Tests: per-category regression tests using a shared assertSingleDropOp helper and a table-driven pattern for the trivial categories. Function drops have dedicated subtests for DiffDrop, overload disambiguation, DiffModify via return-type change (with precondition assertions to prevent vacuous passes), and body-only changes. No existing tests touched. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…erential test Add the remaining drop producers to DropsFromDiff, completing coverage of all 21 drop-emission sites across omni's generate*DDL functions: - dropsForTableRecreates (migration_partition.go — RelKind/inheritance change) - dropsForColumns (migration_column.go — DiffDrop) - dropsForCheckCascades (migration_column.go — CHECK cascade from type change) - dropsForConstraints (migration_constraint.go — DiffDrop + DiffModify) - dropsForViews (migration_view.go — 5 emission sites including matview modify) - dropsForTriggers (migration_trigger.go — DiffDrop + DiffModify) - dropsForDependentViews (migration.go — BFS over from.deps for transitive view cascade) The dependent-view cascade is the most complex producer: it walks the unexported catalog dep graph to find views that transitively depend on tables undergoing column type changes, column drops, RelKind changes, or inheritance changes, then deduplicates against ops already emitted by dropsForViews. Adds a differential test that builds a non-trivial fixture (schema drop, column type change with CHECK cascade, matview modification, function drop, enum drop, sequence drop, trigger drop, dependent-view cascade), runs both GenerateMigration and DropsFromDiff on the same input, and asserts the drop subsets match field-by-field (12 ops across 8 categories, zero mismatches). This test is the primary correctness gate preventing the duplicated decide logic from drifting. Full omni test suite passes (go test ./... -count=1 -short), race detector clean on pg/catalog. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…fix doc comment
Addresses review findings:
1. Add three missing dropsForX helpers that GenerateMigration covers but
DropsFromDiff previously omitted:
- dropsForExtensions (migration_extension.go — DiffDrop)
- dropsForIndexes (migration_index.go — DiffDrop + DiffModify, skipping
constraint-backed indexes)
- dropsForPolicies (migration_policy.go — DiffDrop + DiffModify when
CmdType or Permissive changed)
2. Fix DropsFromDiff doc comment: document Warning as a third difference
(was "two differences", now correctly says "three differences").
3. Broaden the differential test fixture to include extensions, explicit
indexes, and RLS policies. The test now compares 15 drop ops across
11 categories with zero mismatches.
Per-category regression tests added for all three new producers.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address code quality findings: 1. Collapse identical DiffDrop/DiffModify case bodies in dropsForConstraints and dropsForTriggers — both arms produce the same drop op (modify = DROP old + ADD new; we only emit the drop). 2. Extract dropViewOp helper (mirrors dropFunctionOp pattern) to replace 5 identical OpDropView struct literals across dropsForViews and dropsForDependentViews. 3. Add explanatory comment on dropsForConstraints documenting why OpDropConstraint has two different field conventions in the same file (dropsForCheckCascades puts table name in ObjectName with PhaseMain; dropsForConstraints puts constraint name in ObjectName with ParentObject=tableName and PhasePre). Both mirror their respective omni sources faithfully. Net -44 lines of production code. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds a defensive nil check on the DiffDrop branch of dropsForIndexes, matching the pattern used by every other dropsForX helper. The omni source (migration_index.go) also lacks this check, so this is not a correctness fix — purely a consistency improvement. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
I don't know how useful this will be, but since I had the agents running it for a customer problem (that had a different root cause), I'll just leave it here for reference in the future in case we need it later. |
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
1 participant
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.
Problem
A Bytebase Cloud customer running
bytebase-action check --declarativeagainst a ~650-file exported schema gets a consistent 504 (OOMKill) on a 2 GiB Cloud Run pod. Root cause:pgSDLDropAdvicesin Bytebase callscatalog.GenerateMigrationto build a full migration plan, then throws away everything except the drop ops. At scale (~20k table-equivalent metadata objects),GenerateMigrationtakes ~46 seconds and allocates heavily — this is the dominant cost that pushes the pod past its memory limit.The drop-advice function only needs to know what will be dropped. It doesn't need DDL text, topological ordering, or any of the CREATE/ALTER ops that make up the bulk of
GenerateMigration's work.Measured phase timings (from a standalone benchmark at customer scale)
catalog.LoadSDL(current)pgSDLDropAdvicesGenerateMigrationaccounts for ~46s of the 60.6s total at 20k tables (the rest isLoadSDL).Solution
Add a new exported function:
It returns the exact same set of destructive
MigrationOps thatGenerateMigrationwould produce for the same inputs, with three documented differences:GenerateMigration.GenerateMigrationpopulates warnings on some destructive ops (e.g., table recreates);DropsFromDiffomits them.(SchemaName, ObjectName, ParentObject, Type)for determinism, not topologically — the expensive dependency-driven topo sort is skipped.All other metadata fields (
Type,SchemaName,ObjectName,ParentObject,Phase,ObjType,ObjOID,Priority) are populated identically to whatGenerateMigrationsets.Coverage
18
dropsForXhelpers covering everyDrop*op type inGenerateMigration:OpDropSchemamigration_schema.goOpDropExtensionmigration_extension.goOpDropTypemigration_enum.goresolveTypeOIDByNameOpDropTypemigration_domain.goentry.From.TypeOIDOpDropTypemigration_range.goOpDropTypemigration_composite.goObjType='r'(composites are relations)OpDropSequencemigration_sequence.goisIdentitySequencefilter preservedOpDropFunctionmigration_function.goentry.Identityfor overload disambiguation;DiffModify+signatureChangedOpDropTablemigration_table.go'r'/'p'filterOpDropTablemigration_partition.goOpDropColumnmigration_column.goOpDropConstraintmigration_column.goOpDropConstraintmigration_constraint.goDiffDrop+DiffModify; skipsConstraintTriggerOpDropViewmigration_view.goOpDropIndexmigration_index.goOpDropTriggermigration_trigger.goDiffDrop+DiffModifyOpDropPolicymigration_policy.goDiffModifyonly whenCmdType/PermissivechangedOpDropViewmigration.gofrom.depsfor transitive view cascadeWhy duplicate logic instead of refactoring
generate*DDL?A refactor that splits each
generate*DDLinto decide + emit would touch 11 existing files (~1k lines of churn). This PR is a low-risk additive change: two new files, zero modifications to existing code. The differential test pins both paths together so drift fails loudly. Once validated in production, a follow-up can fold the duplication.Test plan
DiffModifypath tests to prevent vacuous passesTestDropsFromDiff_DifferentialAgainstGenerateMigration) — builds a non-trivial fixture, runs bothGenerateMigrationandDropsFromDiff, compares 15 drop ops across 11 categories field-by-field. Zero mismatches.go test -race)go test ./... -count=1 -short)Caller (Bytebase side)
Once this lands, a small follow-up in
bytebase/bytebaserewirespgSDLDropAdvicesinbackend/plugin/schema/pg/sdl_migration_omni.go(~5 lines changed) to callDropsFromDiffinstead ofbuildMigrationPlan. The existingextractDropAdvicesFromPlanhelper works unchanged sinceDropsFromDiffreturns*MigrationPlan.🤖 Generated with Claude Code