fix(pg-delta): order view drops before column type rewrites

[xmh1011]

Refs #263

This is the next small fix for issue #263, scoped to Case 2: ALTER COLUMN ... TYPE can fail when an existing dependent relation still references the old column definition while the same plan is trying to drop and recreate that dependent relation.

What changed

• AlterTableAlterColumnType now declares the rewritten column through the ordering-only invalidates getter. buildGraphData stays generic and folds invalidates into the drop-phase producer set, so existing pg_depend rows can order dependent teardown before the in-place rewrite without changing Change.drops, filtering semantics, fingerprints, or serialization.
• View and materialized-view replacement creates now reuse shared create helpers, so replacement paths replay owner, comments, security labels, and ACL adjustments instead of emitting bare CREATE statements.
• The dropped-target privilege filter now distinguishes pure drops from direct replacements. For a replacement, the same stable id is both dropped and created, so privilege ALTERs target the newly created object and must survive the filter. This covers both replayed GRANTs and REVOKE / REVOKE GRANT OPTION changes generated to subtract privileges inherited from ALTER DEFAULT PRIVILEGES at create time.
• The changeset mentions the ordering fix and the replacement metadata/ACL preservation behavior.

RED evidence

• The original focused PG17 integration regression failed because the generated SQL put ALTER TABLE public.alter_column_type_view_dependent_users ALTER COLUMN age TYPE integer ... before DROP VIEW public.alter_column_type_view_dependent_user_ages. Manual apply reproduced PostgreSQL error 0A000: cannot alter type of a column used by a view or rule.
• The dependent-view metadata regression failed because the generated replacement plan recreated the view and comment but omitted the branch GRANT SELECT.
• The materialized-view metadata regression failed because the replacement plan recreated the materialized view but did not emit COMMENT ON MATERIALIZED VIEW / the dependent grant (commentMatviewIdx was -1).
• The final review pass found a direct replacement ACL gap: when a recreated view inherits SELECT from ALTER DEFAULT PRIVILEGES but the branch model wants no reader ACL, catalog.diff.test.ts failed with Expected: true / Received: false for RevokeViewPrivileges.

Local validation

• Unit-only targeted run for catalog.diff.test.ts, view.diff.test.ts, materialized-view.diff.test.ts, expand-replace-dependencies.test.ts, and sort-changes.test.ts: 32 pass / 0 fail.
• Final focused catalog.diff.test.ts rerun after the ACL filter/comment updates: 2 pass / 0 fail.
• PG17 focused integration rerun: alter-table-operations.test.ts + materialized-view-operations.test.ts with --test-name-pattern "change column type after dropping dependent view|restore materialized view metadata": 3 pass / 0 fail.
• ./node_modules/.bin/bun run format-and-lint:fix: passed.
• ./node_modules/.bin/bun run check-types: passed.
• ./node_modules/.bin/bun run knip --fix: passed; knip only reported the existing configuration hints.

Local integration validation used PGDELTA_SKIP_DUMMY_SECLABEL_BUILD=1 to avoid rebuilding the local dummy security-label image; CI should keep security-label coverage enabled.

Case 1 from #263 remains intentionally covered by the separate follow-up PR.

[changeset-bot]

🦋 Changeset detected

Latest commit: 4e8172e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@supabase/pg-delta	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[xmh1011]

Follow-up from deeper issue #263 validation:

I found one adjacent case in the same column-rewrite dependent-relation family, so I added it to this PR rather than opening a new PR. A materialized view depending on a column whose type is rewritten was already being dropped and recreated, but diffMaterializedViews emitted a bare CreateMaterializedView, so branch metadata carried on the model was not replayed after replacement.

Fix added here:

• RED integration regression for DROP MATERIALIZED VIEW -> ALTER COLUMN TYPE -> CREATE MATERIALIZED VIEW with COMMENT ON MATERIALIZED VIEW and GRANT SELECT.
• diffMaterializedViews now reuses the materialized-view create metadata path for non-alterable replacements.
• expandReplaceDependencies also reuses that helper when it has to synthesize a materialized-view create.

RED evidence: the focused PG17 regression failed after six retry attempts with commentMatviewIdx equal to -1.

Validation run locally:

• materialized-view.diff.test.ts: 9 pass
• expand-replace-dependencies.test.ts: 7 pass
• PGDELTA_TEST_POSTGRES_VERSIONS=17 ... bun run test --test-name-pattern "restore materialized view metadata" tests/integration/materialized-view-operations.test.ts: 1 pass
• PGDELTA_TEST_POSTGRES_VERSIONS=17 ... bun run test tests/integration/materialized-view-operations.test.ts: 9 pass
• PGDELTA_TEST_POSTGRES_VERSIONS=17 ... bun run test --test-name-pattern "change column type after dropping dependent view" tests/integration/alter-table-operations.test.ts: 2 pass
• bun run format-and-lint:fix && bun run check-types && bun run knip --fix: passed (knip emitted existing configuration hints only)

[xmh1011]

One extra local-only validation after the materialized-view metadata fix: I added a deep probe for the cascade-reach risk mentioned in issue #263.

Scenario covered locally:

• populated materialized view over the rewritten column (WITH DATA);
• dependent unique index on the materialized view;
• downstream view over the materialized view.

The PG17 generated plan drops the downstream view and index before DROP MATERIALIZED VIEW, performs the ALTER COLUMN TYPE, recreates the materialized view with WITH DATA, then recreates the index and downstream view. The focused probe passed, and the full local deep-validation file is now 6 pass / 0 fail.

This was kept on a local validation branch only; no extra remote test branch was pushed.

[avallete]

Reviewed this and it's correct — the diagnosis (rewrite routed to the drop phase but missing a producer for the column, so the main-catalog pg_depend edge can't order the dependent DROP VIEW ahead of it) is spot on, and the RED→GREEN evidence holds up.

One design suggestion on where the anchor lives. The fix puts an instanceof AlterTableAlterColumnType check inside buildGraphData, which was previously the only file in src/core/sort/ with no coupling to a concrete change subtype — it's the generic graph builder. The name dropPhase-style anchoring also describes the sorter mechanism rather than the fact about the change.

An alternative that keeps graph-builder.ts generic: make it a first-class fact on the change. Add an invalidates getter to BaseChange (sibling to creates/drops/requires) meaning "I mutate these stable ids in place — the object keeps its identity, but dependents bound to the old definition must be dropped before me and rebuilt after." AlterTableAlterColumnType returns its column; buildGraphData folds invalidates into the drop-phase producer set right next to drops (no instanceof, no import from objects/**/changes/). It's ordering-only — invalidates deliberately does not feed Change.drops, so phase assignment, filtering, fingerprints, and serialization are all unchanged, and recreation order needs no help because the create phase runs entirely after the drop phase.

I pushed that variant as #278 so you can compare side by side — same tests and behavior, the difference is purely that the knowledge sits on the change class instead of in the generic builder. Either is fine functionally; flagging it because the generic-builder coupling is the kind of thing that tends to accrete more instanceofs over time.

[xmh1011]

Thanks @avallete, this design point makes sense.

I updated this PR to adopt the invalidates approach from #278 while keeping the additional issue #263 coverage that had already been added here:

• BaseChange now exposes an ordering-only invalidates getter.
• AlterTableAlterColumnType reports the rewritten column via invalidates.
• graph-builder.ts no longer imports or checks AlterTableAlterColumnType; it only folds change.invalidates into the drop-phase producer set.
• The dependent view metadata and materialized-view metadata regressions/fixes remain in this PR.

Validation after the update:

• Unit-only targeted run: sort-changes.test.ts, materialized-view.diff.test.ts, expand-replace-dependencies.test.ts — 20 pass / 0 fail.
• PG17 integration: alter-table-operations.test.ts + materialized-view-operations.test.ts — 32 pass / 0 fail.
• bun run format-and-lint:fix && bun run check-types && bun run knip --fix passed; knip only reported the existing configuration hints.
• GitHub Lint Pull Request passed on new head f82dea8.

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/supabase/pg-toolbelt/@supabase/pg-delta@273

npm i https://pkg.pr.new/supabase/pg-toolbelt/@supabase/pg-topo@273

commit: 4e8172e

[avallete]

Should document the new primitive in the CLAUDE.md for later work something like:

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

Overall a bit more comments around the code changes / additions for context might be useful.

[xmh1011]

Follow-up pushed in 8dea6e7.

What changed:

• Documented the new invalidates primitive in packages/pg-delta/CLAUDE.md.
• Added context comments for the three inline review points: AlterTableAlterColumnType.invalidates, replacement-created privilege metadata surviving the dropped-target filter, and buildCreateViewReplacementChanges.
• Fixed the PG15 CI failure in the two dependent-view column rewrite tests by avoiding a version-sensitive pg_get_viewdef inline snapshot and asserting the stable behavior instead: DROP VIEW -> ALTER COLUMN TYPE -> CREATE VIEW, plus metadata replay ordering for the metadata case.

Local validation after the change:

• PGDELTA_SKIP_DUMMY_SECLABEL_BUILD=1 PGDELTA_TEST_POSTGRES_VERSIONS=15 bun run --cwd packages/pg-delta test --test-name-pattern "change column type after dropping dependent view" tests/integration/alter-table-operations.test.ts — 2 pass / 0 fail. This reruns the two PG15 tests that failed in CI shard 4.
• PGDELTA_SKIP_DUMMY_SECLABEL_BUILD=1 PGDELTA_TEST_POSTGRES_VERSIONS=15,17,18 bun run --cwd packages/pg-delta test --test-name-pattern "change column type after dropping dependent view" tests/integration/alter-table-operations.test.ts — 6 pass / 0 fail.
• PGDELTA_SKIP_DUMMY_SECLABEL_BUILD=1 PGDELTA_TEST_POSTGRES_VERSIONS=15,17,18 bun run --cwd packages/pg-delta test tests/integration/alter-table-operations.test.ts — 69 pass / 0 fail.
• Unit-only targeted run for sort-changes.test.ts, materialized-view.diff.test.ts, and expand-replace-dependencies.test.ts — 20 pass / 0 fail.
• bun run format-and-lint:fix && bun run check-types && bun run knip --fix passed; knip still reports only the existing configuration hints.

I also replied directly on the three inline threads with the specific change made for each one.

[avallete]

The review-feedback commit (8dea6e7) resolves all four threads cleanly — comments are accurate and the invalidates primitive is now documented in CLAUDE.md. Verified locally: 1064 core unit tests pass and tsc --noEmit is clean.

Leaving a few non-blocking follow-ups inline (view-path duplication, a missing regression for the privilege-filter guard, and a changeset that undersells the PR). None of these block the fix, which is correct and well-scoped.

---

Generated by Claude Code

[xmh1011]

Follow-up pushed in 4d9c29d.

Addressed the three non-blocking review follow-ups:

• extracted a shared buildCreateViewChanges helper from view.diff.ts and reused it from expandReplaceDependencies, matching the materialized-view replacement path and removing duplicated view metadata replay logic;
• added catalog.diff.test.ts coverage for a direct view replacement with a branch GRANT, so the dropped-target privilege filter guard is now covered;
• expanded the changeset text to mention both dependent-view rewrite ordering and view/materialized-view metadata preservation during replacement.

RED evidence for the new guard regression: after adding the test, temporarily removing the creates guard made it fail with Expected: true / Received: false for GrantViewPrivileges; restoring the guard made it pass.

Validation after the final files:

• Unit-only targeted run: catalog.diff.test.ts, view.diff.test.ts, expand-replace-dependencies.test.ts, materialized-view.diff.test.ts — 28 pass / 0 fail.
• PG17 focused integration: alter-table-operations.test.ts + materialized-view-operations.test.ts with --test-name-pattern "change column type after dropping dependent view|restore materialized view metadata" — 3 pass / 0 fail.
• bun run format-and-lint:fix && bun run check-types && bun run knip --fix passed; knip only reported the existing configuration hints.

I also replied directly on each inline thread.

[xmh1011]

Follow-up from the final review pass on this PR:

I found one more case in the same replacement-created ACL metadata area, so I kept it in #273 rather than opening a new PR. The previous guard preserved replacement GRANTs, but a direct replacement can also need a REVOKE after CREATE when the recreated view inherits privileges from ALTER DEFAULT PRIVILEGES and the branch model wants those ACLs absent.

Pushed 4e8172e:

• the dropped-target privilege filter now identifies direct replacements by target stable id (DROP + CREATE for the same id) and preserves all privilege ALTERs for those replacements, including GRANT, REVOKE, and REVOKE GRANT OPTION;
• added a RED regression in catalog.diff.test.ts for the default-privilege subtraction case;
• expanded the changeset to mention ACL adjustments and clarified the code comments around pure drops vs replacements.

Validation after the change:

• RED before fix: catalog.diff.test.ts failed with Expected: true / Received: false for RevokeViewPrivileges.
• Unit-only targeted run for catalog.diff.test.ts, view.diff.test.ts, materialized-view.diff.test.ts, expand-replace-dependencies.test.ts, and sort-changes.test.ts: 32 pass / 0 fail.
• PG17 focused integration for the dependent view/materialized-view column rewrite cases: 3 pass / 0 fail.
• format-and-lint:fix, check-types, and knip --fix passed; knip only reported the existing configuration hints.