Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/cool-windows-worry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@supabase/pg-delta": patch
---

Order dependent view drops before column type rewrites.
3 changes: 2 additions & 1 deletion packages/pg-delta/CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,9 @@ Keep cycle handling split by the scope of information it needs:
- **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.
- **In-place mutations that invalidate dependents declare `invalidates`, not a graph hack**. When a change keeps an object's identity but rewrites it so dependents bound to the old definition must be dropped before it and rebuilt after (the canonical case is `AlterTableAlterColumnType`, whose `ALTER COLUMN … TYPE` forces a PostgreSQL table rewrite), override the `invalidates` getter on the change (sibling to `creates`/`drops`/`requires` in `base.change.ts`) to return the affected stable id. `buildGraphData` folds `invalidates` into the drop-phase producer set exactly like `drops`, so the existing `pg_depend` edges order each dependent's teardown ahead of the mutation. This is ordering-only: `invalidates` does NOT feed `Change.drops`, so phase assignment (`getExecutionPhase`), filtering, fingerprints, and serialization are unchanged, and recreation order needs no help because the create phase always runs after the entire drop phase. Prefer this over adding a change-type `instanceof` to the otherwise generic `graph-builder.ts`.

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.
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; if a change mutates an object in place such that its dependents must be torn down first, it declares `invalidates`.

## Key Concepts

Expand Down
23 changes: 23 additions & 0 deletions packages/pg-delta/src/core/objects/base.change.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,29 @@ export abstract class BaseChange {
return [];
}

/**
* Stable identifiers this change invalidates in place.
*
* Unlike `drops`, the object keeps its identity — it is neither removed nor
* recreated. But an in-place mutation (for example `ALTER COLUMN ... TYPE`,
* which forces a PostgreSQL table rewrite) invalidates everything bound to
* the old definition, so dependents must be dropped before this change and
* rebuilt after it — the same ordering a real drop-and-recreate would demand.
*
* The sorter consumes this for ordering only: in the drop phase the
* invalidated ids act as producers, so the catalog's existing `pg_depend`
* edges order each dependent's teardown ahead of this change. It deliberately
* does NOT feed `drops`, so phase assignment, filtering, fingerprints, and
* serialization are unchanged. Recreation order needs no help here — the
* create phase always runs after the entire drop phase.
*
* Defaults to an empty array. Override in subclasses that mutate an object in
* place in a way that invalidates its dependents.
*/
get invalidates(): string[] {
return [];
}

/**
* Serialize the change into a single SQL statement.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,15 @@ export class AlterTableAlterColumnType extends AlterTableChange {
];
}

// ALTER COLUMN ... TYPE rewrites the column in place. The column keeps its
// identity, but anything bound to its old type (views, rules, etc.) must be
// dropped before the rewrite and rebuilt after, so report it as invalidated.
get invalidates() {
return [
stableId.column(this.table.schema, this.table.name, this.column.name),
];
}

serialize(_options?: SerializeOptions): string {
// previousColumn is optional so direct serializer tests/fixtures can keep
// emitting canonical ALTER TYPE SQL without forcing a USING expression.
Expand Down
8 changes: 8 additions & 0 deletions packages/pg-delta/src/core/sort/graph-builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,17 @@ export function buildGraphData(
(changeItem) => {
const createdIds = new Set<string>(changeItem.creates);
if (options.invert) {
// Drop phase: a change's dropped ids act as producers so each
// dependent's teardown is ordered before this change.
for (const droppedId of changeItem.drops ?? []) {
createdIds.add(droppedId);
}
// An in-place mutation (e.g. ALTER COLUMN ... TYPE) keeps the object
// but invalidates its dependents, so for ordering it behaves exactly
// like a drop of that id: dependents must be torn down first.
for (const invalidatedId of changeItem.invalidates) {
createdIds.add(invalidatedId);
}
}
return createdIds;
},
Expand Down
74 changes: 73 additions & 1 deletion packages/pg-delta/src/core/sort/sort-changes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,15 @@ import type { Change } from "../change.types.ts";
import type { PgDepend } from "../depend.ts";
import { AlterPublicationDropTables } from "../objects/publication/changes/publication.alter.ts";
import { Publication } from "../objects/publication/publication.model.ts";
import { AlterTableDropConstraint } from "../objects/table/changes/table.alter.ts";
import {
AlterTableAlterColumnType,
AlterTableDropConstraint,
} from "../objects/table/changes/table.alter.ts";
import { DropTable } from "../objects/table/changes/table.drop.ts";
import { Table } from "../objects/table/table.model.ts";
import { CreateView } from "../objects/view/changes/view.create.ts";
import { DropView } from "../objects/view/changes/view.drop.ts";
import { View } from "../objects/view/view.model.ts";
import { sortChanges } from "./sort-changes.ts";

const baseTableProps = {
Expand Down Expand Up @@ -142,6 +148,29 @@ function table(
});
}

function view(name: string, columns = [integerColumn("id", 1)]) {
return new View({
schema: "public",
name,
definition: "SELECT id FROM users",
row_security: false,
force_row_security: false,
has_indexes: false,
has_rules: true,
has_triggers: false,
has_subclasses: false,
is_populated: true,
replica_identity: "d",
is_partition: false,
options: null,
partition_bound: null,
owner: "postgres",
comment: null,
columns,
privileges: [],
});
}

async function catalogWithDepends(depends: PgDepend[]) {
const base = await createEmptyCatalog(170000, "postgres");
// oxlint-disable-next-line typescript/no-misused-spread
Expand All @@ -159,6 +188,49 @@ function changeLabel(change: Change) {
}

describe("sortChanges", () => {
test("orders dependent view drop before drop-phase column type rewrite", async () => {
const branchTable = table("users");
const mainColumn = {
...integerColumn("age", 4),
data_type: "numeric",
data_type_str: "numeric",
};
const branchColumn = integerColumn("age", 4);
const dependentView = view("user_ages", [
integerColumn("id", 1),
mainColumn,
]);
const recreatedView = view("user_ages", [
integerColumn("id", 1),
branchColumn,
]);
const changes: Change[] = [
new AlterTableAlterColumnType({
table: branchTable,
column: branchColumn,
previousColumn: mainColumn,
}),
new DropView({ view: dependentView }),
new CreateView({ view: recreatedView }),
];
const mainCatalog = await catalogWithDepends([
{
dependent_stable_id: dependentView.stableId,
referenced_stable_id: "column:public.users.age",
deptype: "n",
},
]);
const branchCatalog = await catalogWithDepends([]);

const sorted = sortChanges({ mainCatalog, branchCatalog }, changes);

expect(sorted.map(changeLabel)).toEqual([
"DropView",
"AlterTableAlterColumnType",
"CreateView",
]);
});

test("breaks publication FK-chain constraint-drop cycle with one dropped table", async () => {
const labs = table("labs", [uniqueConstraint("unique_lab_id", "id")]);
const posts = table("posts", [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* Integration tests for PostgreSQL ALTER TABLE operations.
*/

import { describe, test } from "bun:test";
import { describe, expect, test } from "bun:test";
import type { Change } from "../../src/core/change.types.ts";
import { POSTGRES_VERSIONS } from "../constants.ts";
import { withDb } from "../utils.ts";
Expand Down Expand Up @@ -107,6 +107,68 @@ for (const pgVersion of POSTGRES_VERSIONS) {
}),
);

test(
"change column type after dropping dependent view",
withDb(pgVersion, async (db) => {
await roundtripFidelityTest({
mainSession: db.main,
branchSession: db.branch,
initialSetup: `
CREATE TABLE public.alter_column_type_view_dependent_users (
id integer PRIMARY KEY,
age numeric
);

CREATE VIEW public.alter_column_type_view_dependent_user_ages AS
SELECT id, age
FROM public.alter_column_type_view_dependent_users
WHERE age > 0;
`,
testSql: `
DROP VIEW public.alter_column_type_view_dependent_user_ages;

ALTER TABLE public.alter_column_type_view_dependent_users
ALTER COLUMN age TYPE integer USING age::integer;

CREATE VIEW public.alter_column_type_view_dependent_user_ages AS
SELECT id, age
FROM public.alter_column_type_view_dependent_users
WHERE age > 0;
`,
assertSqlStatements: (sqlStatements) => {
// pg_get_viewdef output (column qualification, whitespace) varies
// across PostgreSQL builds, so assert the ordering the fix
// guarantees rather than snapshotting the recreated view body: the
// dependent view must be dropped before the in-place column rewrite
// (and recreated after). roundtripFidelityTest already applies this
// SQL, so a wrong order would also fail at apply with 0A000.
const dropViewIndex = sqlStatements.findIndex(
(statement) =>
statement.startsWith("DROP VIEW") &&
statement.includes(
"alter_column_type_view_dependent_user_ages",
),
);
const alterTypeIndex = sqlStatements.findIndex((statement) =>
statement.includes("ALTER COLUMN age TYPE integer"),
);
const createViewIndex = sqlStatements.findIndex(
(statement) =>
statement.startsWith("CREATE VIEW") &&
statement.includes(
"alter_column_type_view_dependent_user_ages",
),
);
expect(dropViewIndex).toBeGreaterThanOrEqual(0);
expect(alterTypeIndex).toBeGreaterThanOrEqual(0);
expect(createViewIndex).toBeGreaterThanOrEqual(0);
expect(dropViewIndex).toBeLessThan(alterTypeIndex);
expect(alterTypeIndex).toBeLessThan(createViewIndex);
},
});
}),
);

test(
"change column type to enum with default",
withDb(pgVersion, async (db) => {
Expand Down
Loading