Skip to content

fix: skip FK constraint drop when referenced table is dropped with CASCADE (#382)#383

Merged
tianzhou merged 2 commits intomainfrom
fix/issue-382-drop-table-cascade-constraint
Mar 30, 2026
Merged

fix: skip FK constraint drop when referenced table is dropped with CASCADE (#382)#383
tianzhou merged 2 commits intomainfrom
fix/issue-382-drop-table-cascade-constraint

Conversation

@tianzhou
Copy link
Copy Markdown
Contributor

Summary

When dropping a table with CASCADE, PostgreSQL automatically removes FK constraints on other tables that reference it. The diff generator was also emitting explicit ALTER TABLE ... DROP CONSTRAINT for these same FKs, causing constraint "..." does not exist errors during apply.

The fix filters out FK constraint drops in generateAlterTableStatements when the referenced table is already being dropped with CASCADE.

Fixes #382

Test plan

  • Added testdata/diff/create_table/issue_382_drop_table_cascade_constraint/ test case
  • PGSCHEMA_TEST_FILTER="create_table/issue_382" go test -v ./internal/diff -run TestDiffFromFiles
  • PGSCHEMA_TEST_FILTER="create_table/" go test -v ./cmd -run TestPlanAndApply
  • All create_table/ and dependency/ diff tests pass (no regressions)

🤖 Generated with Claude Code

…SCADE (#382)

When dropping a table with CASCADE, PostgreSQL automatically removes FK
constraints on other tables that reference it. The diff generator was also
emitting explicit ALTER TABLE DROP CONSTRAINT for these same FKs, causing
"constraint does not exist" errors during apply.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 30, 2026 02:29
@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 30, 2026

Greptile Summary

This PR fixes a duplicate-operation error (#382) where dropping a table with CASCADE also caused pgschema to emit an explicit ALTER TABLE … DROP CONSTRAINT for FK constraints already implicitly removed by PostgreSQL's cascade mechanism, resulting in constraint "…" does not exist errors on apply.

Changes:

  • internal/diff/diff.go: passes d.droppedTables through to generateModifyTablesSQL.
  • internal/diff/table.go: generateModifyTablesSQL builds a droppedTableSet from the dropped tables and passes it to generateAlterTableStatements, which now skips FK constraint drops where the referenced table is already being cascaded away.
  • Test case added under testdata/diff/create_table/issue_382_drop_table_cascade_constraint/ verifying that no explicit DROP CONSTRAINT appears in the plan when the referenced table is dropped with CASCADE.

Findings:

  • The refKey construction on line 671 does not apply the empty-ReferencedSchema → table-schema fallback used consistently everywhere else in the file (shouldDeferConstraint) and in topological.go. In practice, the PostgreSQL inspector always populates ReferencedSchema, so this is not a current-breaking bug — but the missing guard is worth aligning with the established pattern.

Confidence Score: 5/5

Safe to merge; the fix is well-scoped and the one finding is a minor defensive-coding inconsistency that does not affect current behavior.

The only finding is a P2 code-quality suggestion (missing empty-schema fallback) that is dormant in practice because the PostgreSQL inspector always populates ReferencedSchema. The core logic of the fix is correct, the test data is appropriate, and no regression is introduced. Per the confidence guidance, a PR with only P2 findings should score 5.

internal/diff/table.go — the empty-schema fallback inconsistency at line 671

Important Files Changed

Filename Overview
internal/diff/table.go Core fix: skips FK DROP CONSTRAINT when referenced table is in the dropped-tables set; minor inconsistency — missing empty-schema fallback present elsewhere in the file
internal/diff/diff.go Passes d.droppedTables to generateModifyTablesSQL; clean one-line change with no issues
testdata/diff/create_table/issue_382_drop_table_cascade_constraint/diff.sql Expected output contains DROP TABLE CASCADE + DROP COLUMN but no explicit DROP CONSTRAINT — correctly validates the fix
testdata/diff/create_table/issue_382_drop_table_cascade_constraint/plan.json Plan steps correctly omit the FK constraint drop

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[generateModifySQL] -->|passes droppedTables| B[generateModifyTablesSQL]
    B --> C{Build droppedTableSet
schema.table → true}
    C --> D[For each tableDiff]
    D --> E[generateAlterTableStatements
droppedTableSet]
    E --> F{For each DroppedConstraint}
    F -->|FK constraint?| G{ReferencedTable
in droppedTableSet?}
    G -->|Yes - CASCADE handles it| H[skip DROP CONSTRAINT]
    G -->|No| I[emit ALTER TABLE DROP CONSTRAINT]
    F -->|Not FK| I
Loading

Reviews (1): Last reviewed commit: "fix: skip FK constraint drop when refere..." | Re-trigger Greptile

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a diff/apply failure when dropping a referenced table with CASCADE by avoiding redundant ALTER TABLE ... DROP CONSTRAINT statements for FK constraints that PostgreSQL already removes during the cascading drop.

Changes:

  • Pass the set of dropped tables into table-alter SQL generation to suppress redundant FK constraint drops.
  • Skip emitting DROP CONSTRAINT for FKs whose referenced table is being dropped with CASCADE.
  • Add a regression fixture for issue #382 under testdata/diff/create_table/.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

File Description
internal/diff/table.go Builds a dropped-table lookup and skips FK constraint drops when the referenced table is dropped with CASCADE.
internal/diff/diff.go Threads d.droppedTables into generateModifyTablesSQL so alter generation can filter FK drops.
testdata/diff/create_table/issue_382_drop_table_cascade_constraint/* Adds expected plans/SQL for the cascade-drop scenario to prevent regressions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Mirror the pattern used in shouldDeferConstraint and topological sorting
to default empty ReferencedSchema to the owning table's schema.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@tianzhou tianzhou merged commit 0601c76 into main Mar 30, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failure dropping a table and a related constraint, when the constraint is included in the cascading drop of the table

2 participants