fix(pg-delta): enhance cycle-breaking logic (#248)

Author: avallete

.changeset/fix-publication-fk-chain-cycle.md

@@ -0,0 +1,5 @@
+---
+"@supabase/pg-delta": patch
+---
+
+Fix drop-phase cycle breaking when publication table membership removal intersects with dropped foreign-key chains and a referenced constraint drop.

.changeset/fix-replaced-table-dropsequence-cycle.md

@@ -0,0 +1,21 @@
+---
+"@supabase/pg-delta": patch
+---
+
+Fix `DropSequence ↔ DropTable` drop-phase cycle when an owning table is
+promoted to `DropTable + CreateTable` by `expandReplaceDependencies` (for
+example when a referenced enum has a label removed) and the same plan also
+drops the SERIAL sequence because branch no longer carries the owned sequence.
+
+`diffSequences.dropped` short-circuits `DropSequence` only when the owning
+table itself is absent from the branch catalog. When the table survives in
+branch but is later replaced via expansion (table is in `replacedTableIds`),
+the explicit `DROP SEQUENCE` survives into the drop phase alongside the
+expander's `DropTable`, and the bidirectional pg_depend edges between the
+sequence and its owning column close an unbreakable 2-cycle that none of the
+existing dependency-filter / change-injection breakers match.
+
+`normalizePostDiffChanges` now prunes `DropSequence(S)` whenever S is `OWNED
+BY` a column on a table in `replacedTableIds`. The `DROP TABLE` cascade
+already drops the OWNED BY sequence at apply time, so the explicit
+`DROP SEQUENCE` was both redundant and the source of the cycle.

CONTEXT.md

@@ -0,0 +1,60 @@
+# pg-toolbelt
+
+PostgreSQL tooling for comparing schemas, planning migrations, and ordering DDL safely. This context captures the project language used when discussing pg-delta migration planning and dependency-cycle handling.
+
+## Language
+
+**Migration plan**:
+An ordered set of DDL changes that transforms a source database schema into a target database schema.
+_Avoid_: Script, diff output
+
+**Change**:
+A single schema operation emitted by a diff, carrying the stable identifiers it creates, drops, or requires.
+_Avoid_: Statement, when referring to the typed operation before serialization
+
+**Stable identifier**:
+An environment-independent name for a schema object used to connect changes to catalog dependencies.
+_Avoid_: OID
+
+**Dependency cycle**:
+A cycle in the migration-plan dependency graph that prevents topological ordering.
+_Avoid_: Circular diff
+
+**Structural normalization**:
+A deterministic rewrite of the final change list before dependency sorting.
+_Avoid_: Cycle breaker
+
+**Cycle-breaking change injection**:
+A sort-phase rewrite that injects or rebuilds changes after an unbreakable dependency cycle is detected.
+_Avoid_: Post-diff normalization, when the fix is specific to a detected graph cycle
+
+**Publication FK-chain constraint-drop cycle**:
+A dependency cycle where publication membership is being removed for dropped tables, those dropped tables carry a foreign-key chain, and the chain ends at a separately dropped referenced constraint.
+_Avoid_: Publication drop cycle, dropped-table publication membership cycle
+
+**FK constraint-drop injection**:
+Cycle-breaking change injection that creates explicit foreign-key constraint drops and makes table drops stop claiming those constraint stable identifiers.
+_Avoid_: Relaxed publication requirement, when resolving dropped-table publication membership cycles
+
+## Relationships
+
+- A **Migration plan** contains one or more **Changes**.
+- A **Change** names the **Stable identifiers** it creates, drops, or requires.
+- **Structural normalization** happens before dependency sorting and does not inspect a specific cycle path.
+- **Cycle-breaking change injection** happens during dependency sorting and responds to a concrete **Dependency cycle**.
+- A **Publication FK-chain constraint-drop cycle** is resolved by **Cycle-breaking change injection**, not by structural normalization.
+- A **Publication FK-chain constraint-drop cycle** is resolved with **FK constraint-drop injection** while leaving publication membership and referenced-constraint drop changes unchanged.
+- In a **Publication FK-chain constraint-drop cycle**, the terminal referenced-constraint drop table must be part of the publication membership being removed.
+- **FK constraint-drop injection** for a **Publication FK-chain constraint-drop cycle** is cycle-local: inject only FK drops that point to a dropped table in the cycle or to the terminal referenced constraint being dropped.
+- **FK constraint-drop injection** can be shared by multiple cycle breakers; each breaker still owns its own matcher and safety checks.
+
+## Example dialogue
+
+> **Dev:** "Should this publication/table drop issue be handled by structural normalization?"
+> **Domain expert:** "No. The final change list is valid; the problem appears only after dependency sorting detects the specific cycle, so it belongs in cycle-breaking change injection."
+
+## Flagged ambiguities
+
+- "Whole-plan interaction" was used for both **Structural normalization** and **Cycle-breaking change injection**. Resolved: deterministic rewrites of the final change list are structural normalization; rewrites triggered by a concrete unbreakable dependency cycle are cycle-breaking change injection.
+- `AlterTableDropConstraint` was first described as optional in the publication/table drop cycle. Resolved: the observed production cycle is a **Publication FK-chain constraint-drop cycle**, so a separately emitted referenced-constraint drop is part of that specific matcher.
+- Rebuilding `AlterPublicationDropTables` with relaxed requirements was considered for **Publication FK-chain constraint-drop cycles**. Resolved: keep publication membership changes unchanged and break the foreign-key chain with **FK constraint-drop injection**.

packages/pg-delta/CLAUDE.md

@@ -88,11 +88,12 @@ for (const pgVersion of POSTGRES_VERSIONS) {
 Keep cycle handling split by the scope of information it needs:
 
 - **Object-local PostgreSQL semantics stay in `diff*`**. If a single object diff can prove a statement is redundant or invalid on its own, fix it there. Example: `src/core/objects/sequence/sequence.diff.ts` skips `DROP SEQUENCE` when `OWNED BY` means PostgreSQL will already cascade-drop it with the owning table/column.
-- **Whole-plan interactions belong in post-diff normalization**. If the fix only becomes obvious after multiple emitted changes are combined, implement it in `src/core/post-diff-normalization.ts` (`normalizePostDiffChanges`), wired from `src/core/catalog.diff.ts` after raw diffs and `expandReplaceDependencies()`. The pass is the single chokepoint that observes the final `Change[]`, so it catches cross-object effects regardless of whether the relevant change pair was emitted by an object's `diff*` (e.g. `index.diff` for a definition-changed index) or by `expandReplaceDependencies()` (dependency-closure replacement). Current examples: pruning same-table `AlterTableDropColumn` / `AlterTableDropConstraint` changes superseded by an expansion-added `DropTable+CreateTable` pair, deduplicating constraint Add/Validate/Comment on replaced tables, and re-emitting `ALTER TABLE … REPLICA IDENTITY USING INDEX` after a replica-identity index is dropped+recreated.
+- **Deterministic whole-plan rewrites belong in post-diff normalization**. If the final `Change[]` itself should be rewritten before dependency sorting, implement it in `src/core/post-diff-normalization.ts` (`normalizePostDiffChanges`), wired from `src/core/catalog.diff.ts` after raw diffs and `expandReplaceDependencies()`. The pass is the single chokepoint that observes the final `Change[]`, so it catches cross-object effects regardless of whether the relevant change pair was emitted by an object's `diff*` (e.g. `index.diff` for a definition-changed index) or by `expandReplaceDependencies()` (dependency-closure replacement). Current examples: pruning same-table `AlterTableDropColumn` / `AlterTableDropConstraint` changes superseded by an expansion-added `DropTable+CreateTable` pair, deduplicating constraint Add/Validate/Comment on replaced tables, and re-emitting `ALTER TABLE … REPLICA IDENTITY USING INDEX` after a replica-identity index is dropped+recreated.
+- **Unbreakable graph cycles belong in sort-phase change injection**. If the emitted statements are valid but topological sorting discovers a hard dependency cycle that cannot be solved by weak-edge filtering, implement the narrow pattern in `src/core/sort/cycle-breakers.ts` (`tryBreakCycleByChangeInjection`). Existing examples: injecting explicit FK constraint drops for dropped-table FK cycles and rebuilding `AlterTableDropColumn` for publication-column cycles on surviving tables.
 - **`expandReplaceDependencies()` only computes replacement closure**. It may report metadata such as which tables were promoted to replacement pairs, but it should not own unrelated cycle-pruning policy.
 - **`src/core/sort/dependency-filter.ts` is a narrow last resort**. Use it only for safe edge filtering where the emitted statements are already valid and only the graph edge is artificial. Do not extend sort-phase filtering to paper over plans that would still fail at apply time.
 
-Rule of thumb: if the fix needs the full final `Change[]`, it is post-diff; if it needs only one object's semantics, it belongs in that object's `diff*`; if it only removes a graph edge without changing emitted SQL, it belongs in the sort filter.
+Rule of thumb: if the fix changes a valid final `Change[]` before graph construction, it is post-diff; if it reacts to a concrete unbreakable dependency cycle and needs to inject or rebuild changes, it belongs in the sort-phase cycle breakers; if it needs only one object's semantics, it belongs in that object's `diff*`; if it only removes a graph edge without changing emitted SQL, it belongs in the sort filter.
 
 ## Key Concepts
 

packages/pg-delta/src/core/post-diff-normalization.test.ts

@@ -3,6 +3,12 @@ import type { Change } from "./change.types.ts";
 import { CreateIndex } from "./objects/index/changes/index.create.ts";
 import { DropIndex } from "./objects/index/changes/index.drop.ts";
 import { Index, type IndexProps } from "./objects/index/index.model.ts";
+import { CreateSequence } from "./objects/sequence/changes/sequence.create.ts";
+import { DropSequence } from "./objects/sequence/changes/sequence.drop.ts";
+import {
+  Sequence,
+  type SequenceProps,
+} from "./objects/sequence/sequence.model.ts";
 import {
   AlterTableAddConstraint,
   AlterTableChangeOwner,
@@ -304,6 +310,123 @@ describe("normalizePostDiffChanges", () => {
     ).toHaveLength(1);
   });
 
+  describe("DropSequence pruning on replaced tables", () => {
+    const baseSequenceProps: SequenceProps = {
+      schema: "public",
+      name: "project_link_type_id_seq",
+      data_type: "integer",
+      start_value: 1,
+      minimum_value: 1n,
+      maximum_value: 2147483647n,
+      increment: 1,
+      cycle_option: false,
+      cache_size: 1,
+      persistence: "p",
+      owned_by_schema: "public",
+      owned_by_table: "project_link_type",
+      owned_by_column: "id",
+      comment: null,
+      privileges: [],
+      owner: "postgres",
+    };
+
+    test("prunes DropSequence when its OWNED BY table is in replacedTableIds", () => {
+      const replacedTable = new Table({
+        ...baseTableProps,
+        name: "project_link_type",
+        columns: [{ ...integerColumn("id", 1), not_null: true }],
+      });
+      const ownedSequence = new Sequence(baseSequenceProps);
+
+      const dropSequence = new DropSequence({ sequence: ownedSequence });
+      const dropTable = new DropTable({ table: replacedTable });
+      const createTable = new CreateTable({ table: replacedTable });
+
+      const changes: Change[] = [dropSequence, dropTable, createTable];
+
+      const normalized = normalizePostDiffChanges({
+        changes,
+        replacedTableIds: new Set([replacedTable.stableId]),
+      });
+
+      expect(normalized.some((change) => change instanceof DropSequence)).toBe(
+        false,
+      );
+      expect(normalized).toContain(dropTable);
+      expect(normalized).toContain(createTable);
+    });
+
+    test("keeps DropSequence whose OWNED BY table is not in replacedTableIds", () => {
+      const survivingTable = new Table({
+        ...baseTableProps,
+        name: "project_link_type",
+        columns: [{ ...integerColumn("id", 1), not_null: true }],
+      });
+      const ownedSequence = new Sequence(baseSequenceProps);
+
+      const dropSequence = new DropSequence({ sequence: ownedSequence });
+
+      const normalized = normalizePostDiffChanges({
+        changes: [dropSequence],
+        // Different table is being replaced; the sequence's OWNED BY does
+        // not match, so DropSequence must survive.
+        replacedTableIds: new Set([
+          `table:${survivingTable.schema}.unrelated_table` as const,
+        ]),
+      });
+
+      expect(normalized).toContain(dropSequence);
+    });
+
+    test("keeps DropSequence with no OWNED BY when replacedTableIds is non-empty", () => {
+      const orphanSequence = new Sequence({
+        ...baseSequenceProps,
+        owned_by_schema: null,
+        owned_by_table: null,
+        owned_by_column: null,
+      });
+
+      const dropSequence = new DropSequence({ sequence: orphanSequence });
+
+      const normalized = normalizePostDiffChanges({
+        changes: [dropSequence],
+        replacedTableIds: new Set(["table:public.project_link_type" as const]),
+      });
+
+      expect(normalized).toContain(dropSequence);
+    });
+
+    test("keeps unrelated CreateSequence and DropSequence even when its non-owning table is replaced", () => {
+      const sequenceA = new Sequence(baseSequenceProps);
+      const sequenceB = new Sequence({
+        ...baseSequenceProps,
+        name: "unrelated_seq",
+        owned_by_schema: null,
+        owned_by_table: null,
+        owned_by_column: null,
+      });
+
+      const dropOwned = new DropSequence({ sequence: sequenceA });
+      const createUnrelated = new CreateSequence({ sequence: sequenceB });
+
+      const replacedTable = new Table({
+        ...baseTableProps,
+        name: "project_link_type",
+        columns: [{ ...integerColumn("id", 1), not_null: true }],
+      });
+
+      const normalized = normalizePostDiffChanges({
+        changes: [dropOwned, createUnrelated],
+        replacedTableIds: new Set([replacedTable.stableId]),
+      });
+
+      expect(normalized.some((change) => change instanceof DropSequence)).toBe(
+        false,
+      );
+      expect(normalized).toContain(createUnrelated);
+    });
+  });
+
   describe("restoreReplicaIdentityAfterIndexReplace", () => {
     const baseIndexProps: IndexProps = {
       schema: "public",

packages/pg-delta/src/core/post-diff-normalization.ts

@@ -1,6 +1,7 @@
 import type { Change } from "./change.types.ts";
 import { CreateIndex } from "./objects/index/changes/index.create.ts";
 import { DropIndex } from "./objects/index/changes/index.drop.ts";
+import { DropSequence } from "./objects/sequence/changes/sequence.drop.ts";
 import {
   AlterTableAddConstraint,
   AlterTableDropColumn,
@@ -24,12 +25,40 @@ function isSupersededByTableReplacement(
   replacedTableIds: ReadonlySet<string>,
 ): boolean {
   if (
-    !(change instanceof AlterTableDropColumn) &&
-    !(change instanceof AlterTableDropConstraint)
+    change instanceof AlterTableDropColumn ||
+    change instanceof AlterTableDropConstraint
   ) {
-    return false;
+    return replacedTableIds.has(change.table.stableId);
   }
-  return replacedTableIds.has(change.table.stableId);
+
+  // `DropSequence(S)` is superseded when S is OWNED BY a column on a table
+  // that `expandReplaceDependencies` has promoted to `DropTable + CreateTable`
+  // in the same plan. PostgreSQL cascade-drops the OWNED BY sequence as part
+  // of the DROP TABLE, so the explicit DROP SEQUENCE is redundant and — more
+  // importantly — closes an unbreakable `DropSequence ↔ DropTable` cycle in
+  // the drop phase via the bidirectional pg_depend edges between the
+  // sequence and its owning column (`column → sequence` for the DEFAULT
+  // nextval reference, `sequence → column` for the OWNED BY auto-dependency).
+  // The alpha.15 short-circuit in `diffSequences.dropped` only suppresses
+  // `DropSequence` when the owning table itself is gone from `branchTables`;
+  // here the table survives in branch and the replacement is added later by
+  // the expander, so this whole-plan rewrite has to happen post-diff.
+  if (change instanceof DropSequence) {
+    if (
+      !change.sequence.owned_by_schema ||
+      !change.sequence.owned_by_table ||
+      !change.sequence.owned_by_column
+    ) {
+      return false;
+    }
+    const ownedByTableId = stableId.table(
+      change.sequence.owned_by_schema,
+      change.sequence.owned_by_table,
+    );
+    return replacedTableIds.has(ownedByTableId);
+  }
+
+  return false;
 }
 
 /**
@@ -219,6 +248,13 @@ function restoreReplicaIdentityAfterIndexReplace(
  *   `DropTable(T) + CreateTable(T)` pair. Without this, the apply phase
  *   would try to drop a column that no longer exists in the freshly
  *   recreated table.
+ * - Prunes `DropSequence(S)` changes when `S` is `OWNED BY` a column on a
+ *   table promoted to `DropTable + CreateTable` by the expander. The
+ *   `DROP TABLE` cascade drops the sequence at apply time; emitting an
+ *   explicit `DROP SEQUENCE` in the same drop phase both duplicates the
+ *   cascade and forms an unbreakable `DropSequence ↔ DropTable` cycle on
+ *   the bidirectional pg_depend edges between the sequence and the
+ *   owning column.
  * - Dedupes duplicate `AlterTableAddConstraint` /
  *   `AlterTableValidateConstraint` / `CreateCommentOnConstraint` changes
  *   produced when `diffTables()` and `expandReplaceDependencies()` both