fix(pg-delta): recreate rules and triggers for replacement dependencies#285
fix(pg-delta): recreate rules and triggers for replacement dependencies#285xmh1011 wants to merge 49 commits into
Conversation
🦋 Changeset detectedLatest commit: bc2ace3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e57e706451
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3c266759a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ce8b97a6e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5d6b234fe
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d33cf9d0bc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
commit: |
d33cf9d to
2f2100b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ccb4a1c91
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fd4a87d42
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bd3143c47
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
5c6d36e to
a550a34
Compare
There was a problem hiding this comment.
💡 Codex Review
Do not report publication table membership as table drops
When a migration only removes a table from a publication, this getter adds the table stable id to the global droppedIds set; expandReplaceDependencies then treats any table:* drop-only id as a replacement root and walks/rebuilds dependents of that table even though the table itself survives. For a published table with dependent views/rules/etc., a simple ALTER PUBLICATION ... DROP TABLE can therefore synthesize unrelated drop/create DDL and risk losing dependent metadata instead of only changing publication membership.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
a550a34 to
31f8ec4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31f8ec43a9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
31f8ec4 to
52acea9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8368591e80
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09ec2406cb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 710f4397ec
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Final #263 audit update after the latest #285 review fixes. Current branch state:
I did another focused deep pass over the #263 dependency-replacement family after adding the latest fixes in #285. I did not find another unresolved case in the tested replacement/invalidation classes. Coverage now includes:
Fresh verification on the latest #285 tree: cd packages/pg-delta
PGDELTA_SKIP_DUMMY_SECLABEL_BUILD=1 \
PGDELTA_TEST_POSTGRES_VERSIONS=17 \
TESTCONTAINERS_RYUK_DISABLED=true \
../../node_modules/.bin/bun run test --max-concurrency 1 --timeout 120000 \
src/core/expand-replace-dependencies.test.ts \
src/core/sort/sort-changes.test.ts \
src/core/objects/index/changes/index.create.test.ts \
src/core/objects/index/index.diff.test.ts \
src/core/objects/table/table.diff.test.ts \
tests/integration/alter-table-operations.test.ts \
tests/integration/policy-dependencies.test.ts \
tests/integration/rule-operations.test.ts \
tests/integration/trigger-operations.test.ts \
tests/integration/function-operations.test.ts \
tests/integration/index-operations.test.ts \
tests/integration/publication-operations.test.ts \
tests/integration/materialized-view-operations.test.ts \
tests/integration/aggregate-operations.test.ts \
tests/integration/table-function-dependency-ordering.test.ts \
tests/integration/table-operations.test.tsResult: The two skips are the pre-PG17 generated-column-type tests skipped under the PG17-only matrix. There were transient testcontainer connection retries during one alter-table case, but the retry passed and the final suite result was clean. Based on this pass, I think #263 is covered by the merged PRs plus #285, and can be considered fixed once #285 merges. |
8ddf3ce to
c6f805b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6f805bada
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f397f84d4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 775d6af129
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 701790af3a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@avallete @supabase/dev-workflows gentle ping for a code review pass. All current review feedback on this PR has been addressed, and I rechecked the review-thread state today: Could you please review when you have a chance? Once this merges, I think #263 can be considered covered by the merged dependency-replacement work plus this PR. |
RED evidence: Unit: expandReplaceDependencies did not add DropRule for dependent rules when a procedure signature changed or a column was invalidated. Integration PG17: rule rewrite failed with PostgreSQL 0A000 cannot alter type of a column used by a view or rule. Integration PG17: trigger rewrite failed with PostgreSQL 0A000 cannot alter type of a column used in a trigger definition. Signed-off-by: xiaominghao <xiaominghao@baidu.com>
Promote rule and trigger dependents from pg_depend when replacement roots or invalidated objects require dependent rewrite objects to be removed before the parent change. GREEN evidence: Unit: expand-replace-dependencies.test.ts passed 10/10. Integration PG17: rule and trigger column rewrite regressions passed. Integration PG17: rule and trigger function signature regressions passed. format-and-lint:fix, check-types, and knip --fix exited 0. Signed-off-by: xiaominghao <xiaominghao@baidu.com>
Signed-off-by: xiaominghao <xiaominghao@baidu.com>
RED evidence:\n- expandReplaceDependencies > promotes aggregate dependents when a column invalidation rebuilds a SQL routine failed because no DropAggregate was produced.\n- expandReplaceDependencies > drops constraints that depend on routines rebuilt from column invalidation failed because no AlterTableDropConstraint was produced.\n- table.diff > postgres 17 rebuilds constrained generated columns when their type changes failed because no AlterTableDropColumn was produced.\n- PG17 integration generated ALTER COLUMN SET EXPRESSION / ALTER COLUMN TYPE for a constrained generated column instead of DROP COLUMN / ADD COLUMN; aggregate and constraint routine dependency tests missed the expected drops.
GREEN evidence:\n- cd packages/pg-delta && ../../node_modules/.bin/bun run test src/core/expand-replace-dependencies.test.ts src/core/objects/table/table.diff.test.ts --test-name-pattern 'promotes aggregate dependents|drops constraints that depend on routines|postgres 17 rebuilds constrained generated' -> 3 pass, 0 fail.\n- cd packages/pg-delta && PGDELTA_SKIP_DUMMY_SECLABEL_BUILD=1 PGDELTA_TEST_POSTGRES_VERSIONS=17 TESTCONTAINERS_RYUK_DISABLED=true ../../node_modules/.bin/bun run test --timeout 120000 tests/integration/table-function-dependency-ordering.test.ts tests/integration/table-operations.test.ts --test-name-pattern 'aggregate depending on invalidated SQL function|constraint depending on invalidated SQL function|postgres 17 generated column type change' -> 3 pass, 0 fail.\n- Broader focused unit suite -> 63 pass, 0 fail.\n- Broader PG17 focused integration suite -> 7 pass, 0 fail.\n- ./node_modules/.bin/bun run format-and-lint:fix && ./node_modules/.bin/bun run check-types && ./node_modules/.bin/bun run knip --fix && git diff --check -> pass (knip config hints only).
RED evidence before the fix: - Unit: expand-replace-dependencies.test.ts failed because view add-drop-only expansion did not emit AlterViewChangeOwner and index rebuild expansion did not emit AlterIndexSetStatistics. - Integration PG17: alter-table-operations.test.ts still emitted SET EXPRESSION AS (NULL::character varying(64)) / ALTER COLUMN TYPE / SET EXPRESSION for a NOT NULL generated column own type change instead of DROP COLUMN / ADD COLUMN.
Replay index statistics and view metadata when column invalidation expansion synthesizes dependent rebuilds, and rebuild PostgreSQL 17 generated columns when NOT NULL constraints make SET EXPRESSION unsafe. GREEN validation: - ../../node_modules/.bin/bun run test src/core/expand-replace-dependencies.test.ts --test-name-pattern 'replays (index statistics|view metadata)' -> 2 pass / 0 fail. - PGDELTA_SKIP_DUMMY_SECLABEL_BUILD=1 PGDELTA_TEST_POSTGRES_VERSIONS=17 TESTCONTAINERS_RYUK_DISABLED=true ../../node_modules/.bin/bun run test tests/integration/alter-table-operations.test.ts --test-name-pattern 'postgres 17 rebuilds not-null generated column for own type changes' -> 1 pass / 0 fail. - ../../node_modules/.bin/bun run test src/core/expand-replace-dependencies.test.ts -> 31 pass / 0 fail. - ./node_modules/.bin/bun run format-and-lint:fix && ./node_modules/.bin/bun run check-types && ./node_modules/.bin/bun run knip --fix && git diff --check -> exit 0; knip reported existing configuration hints only.
RED summaries: - generated-column rebuild left AlterTableAlterColumnType for status_label: expected false, received true. - matview index rebuild omitted AlterIndexSetStatistics: expected true, received false. - FK-backed standalone index rebuild produced no dependent FK drop/add: dropConstraintIdx was -1.
GREEN verification: - focused expandReplaceDependencies RED tests now pass. - PG17 table integration passes generated-column superseded type alter and FK-backed index replacement cases. - PG17 materialized-view integration passes index statistics replay coverage.
Signed-off-by: xiaominghao <xiaominghao@baidu.com>
Generated-column reset now casts the temporary NULL expression to the current column type, so PostgreSQL 17 incompatible generated own-type changes no longer emit NULL::new_type before ALTER COLUMN TYPE. Generated-column and standalone index rebuild expansion now reapplies REPLICA IDENTITY USING INDEX without duplicating an existing setter, and constraint rebuilds replay backing index statistics after the constraint recreates its owned index. RED evidence: - table.diff incompatible generated own-type regression failed with NULL::text instead of NULL::integer. - generated-column rebuild replica identity regression failed with replicaIdentityIdx = -1 after CreateIndex. - constraint-owned backing index statistics regression failed with statisticsIdx = -1 after AddConstraint. - standalone index rebuild replica identity regression failed with 0 AlterTableSetReplicaIdentity changes. GREEN validation: - table.diff/table.alter focused generated + replica identity matrix: 8 pass / 0 fail. - expand-replace-dependencies.test.ts: 35 pass / 0 fail. - check-types, format-and-lint:fix, knip --fix, and git diff --check exited 0; knip reported existing config hints only. Signed-off-by: xiaominghao <xiaominghao@baidu.com>
RED evidence: - missing branch-table regression fails with TypeError: undefined is not an object (evaluating 'branchTable.schema'). - constraint-owned replica identity regression fails with Expected length: 1 / Received length: 0 for AlterTableSetReplicaIdentity. Signed-off-by: xiaominghao <xiaominghao@baidu.com>
Skip constraint backing-index metadata replay when the branch table or branch constraint no longer exists, avoiding the missing branch-table dereference during drop-only constraint expansion. Also reapply REPLICA IDENTITY USING INDEX after constraint-owned backing indexes are recreated, reusing the existing replacement metadata de-duplication path so pre-existing setters are not duplicated. GREEN validation: - PGDELTA_SKIP_DUMMY_SECLABEL_BUILD=1 PGDELTA_TEST_POSTGRES_VERSIONS=17 TESTCONTAINERS_RYUK_DISABLED=true ../../node_modules/.bin/bun run test src/core/expand-replace-dependencies.test.ts --test-name-pattern 'keeps drop-only constraint dependents|reapplies replica identity when constraint rebuilds' -> 2 pass / 0 fail. - PGDELTA_SKIP_DUMMY_SECLABEL_BUILD=1 PGDELTA_TEST_POSTGRES_VERSIONS=17 TESTCONTAINERS_RYUK_DISABLED=true ../../node_modules/.bin/bun run test src/core/expand-replace-dependencies.test.ts -> 37 pass / 0 fail. - ./node_modules/.bin/bun run format-and-lint:fix && ./node_modules/.bin/bun run check-types && ./node_modules/.bin/bun run knip --fix && git diff --check -> pass; knip reported existing configuration hints only. Signed-off-by: xiaominghao <xiaominghao@baidu.com>
c23e7d1 to
a055809
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a055809995
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
RED evidence:\n- table.alter unit run failed because AlterTableSetCluster is not exported.\n- expand replacement focused unit run failed: quoted stable ids did not emit generated-column/publication rebuilds; clustered index replay was missing (clusterIdx -1 after CreateIndex).\n- PG17 generated-column integration failed because resetGenerated index 0 was not greater than dropIndex 1, proving SET EXPRESSION ran before DROP INDEX.
Preserve CLUSTER markers when replacement rebuilds clustered indexes, parse quoted table stable IDs without splitting inside quoted identifiers, and invalidate generated-column expression resets so dependent indexes drop before the reset. Generated column type rewrites also omit PostgreSQL's forbidden USING clause, and explicit same-id invalidation requirements no longer create cycles between same-column in-place ALTERs. RED evidence: - table.alter serializer regression failed because generated column type rewrite emitted USING computed_name::text. - sort-changes regression failed with AlterTableAlterColumnDropDefault <-> AlterTableAlterColumnType explicit column cycle. - alter-table integration failed before the fix because generated-column index reset ran before DropIndex, then exposed PostgreSQL 42611 for USING on generated columns. Validation: - bun test touched pg-delta unit files from /tmp: table.alter, table.diff, sort-changes, expand-replace-dependencies - PGDELTA_SKIP_DUMMY_SECLABEL_BUILD=1 PGDELTA_TEST_POSTGRES_VERSIONS=17 TESTCONTAINERS_RYUK_DISABLED=true bun run test --max-concurrency 1 --timeout 120000 tests/integration/alter-table-operations.test.ts - PGDELTA_SKIP_DUMMY_SECLABEL_BUILD=1 PGDELTA_TEST_POSTGRES_VERSIONS=17 TESTCONTAINERS_RYUK_DISABLED=true bun run test --max-concurrency 1 --timeout 120000 tests/integration/index-operations.test.ts - bun run format-and-lint:fix - bun run check-types - bun run knip --fix - git diff --check
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00023b2da0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
RED evidence: - expand-replace-dependencies focused test failed with clusterIdx -1: Expected > 3 / Received -1 because no AlterMaterializedViewSetCluster was emitted. - materialized-view alter focused test failed with missing export: AlterMaterializedViewSetCluster not found.
Adds ALTER MATERIALIZED VIEW ... CLUSTER ON support and replays clustered markers for standalone materialized-view indexes recreated during dependency expansion. Validation: - Unit: expand-replace-dependencies.test.ts + materialized-view.alter.test.ts, 43 pass / 0 fail. - Integration: PG17 restore materialized view index when replacing for column type rewrite, 1 pass / 0 fail. - format-and-lint:fix, check-types, knip --fix, and git diff --check passed; knip only reported existing configuration hints.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/supabase/pg-toolbelt/blob/20e3b53f4e9ef26fc46e3595d4b5d1bac6155f73//tmp/pg-toolbelt-review-1466/packages/pg-delta/src/core/objects/table/table.diff.ts#L793-L800
For a PostgreSQL 17 generated column that is nullable and has no constraints, this guard leaves the reset + ALTER COLUMN ... TYPE path in place. The serializer then intentionally omits USING for generated columns, so conversions without an implicit/assignment cast from the old stored type to the new one (for example an integer GENERATED ... column becoming uuid GENERATED ...) still fail during the type alter before the final expression is restored; those cases need to take the drop/add rebuild path instead. Fresh evidence beyond the earlier reset-cast thread: the current guard only checks nullability/constraints and never accounts for no-assignment-cast type changes.
https://github.com/supabase/pg-toolbelt/blob/20e3b53f4e9ef26fc46e3595d4b5d1bac6155f73//tmp/pg-toolbelt-review-1466/packages/pg-delta/src/core/expand-replace-dependencies.ts#L1397
Adding aggregate: here only makes aggregates rebuildable when reached from an invalidated column/routine, but an aggregate whose own argument signature changes gets different old/new stable IDs and is never added to replaceRoots (there is no aggregate name match like procedures, and the drop-only seed omits aggregates). If a surviving view/rule is updated to stop using the old aggregate signature, the plan still runs DROP AGGREGATE in the drop phase before that rewrite object is dropped/recreated, so PostgreSQL rejects the drop due to the old pg_depend edge.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
RED evidence: focused regressions failed before the fix. table.diff expected generated-column drop/add but received reset plus ALTER COLUMN status_id TYPE uuid; expandReplaceDependencies expected a promoted DropView/CreateView for an aggregate signature change but dropViewIdx was -1.
Generated-column type changes on PostgreSQL 17+ now rebuild when the new stored type has no implicit or assignment cast from the old type. Aggregate signature replacements are seeded as replacement roots so surviving rewrite objects are dropped before the old signature is dropped.\n\nGREEN evidence: related unit files passed with 69 pass / 0 fail; PG17 focused integration for generated-column no-cast and aggregate-signature replacement passed with 2 pass / 0 fail. Static checks passed: format-and-lint:fix, check-types, knip --fix, and git diff --check.
|
Addressed the latest Codex Review in Changes:
Regression coverage:
Fresh verification: ./node_modules/.bin/bun test
/Users/xiaominghao/.config/superpowers/worktrees/pg-toolbelt/pr285-sync-main-20260701/packages/pg-delta/src/core/objects/table/table.diff.test.ts
/Users/xiaominghao/.config/superpowers/worktrees/pg-toolbelt/pr285-sync-main-20260701/packages/pg-delta/src/core/expand-replace-dependencies.test.ts
# 69 pass / 0 fail
cd packages/pg-delta
PGDELTA_SKIP_DUMMY_SECLABEL_BUILD=1
PGDELTA_TEST_POSTGRES_VERSIONS=17
TESTCONTAINERS_RYUK_DISABLED=true
../../node_modules/.bin/bun run test --max-concurrency 1 --timeout 120000
tests/integration/alter-table-operations.test.ts
tests/integration/aggregate-operations.test.ts
--test-name-pattern "target type has no assignment cast|old aggregate signature"
# 2 pass / 0 failStatic checks also exited 0: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3d3415859
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
RED evidence: focused regressions failed before the fix. generated domain column changes produced NULL reset plus ALTER COLUMN TYPE instead of drop/add; domain CHECK and domain DEFAULT dependents produced zero AlterDomain drop/restore changes; the sorter did not place AlterDomainDropDefault before DropProcedure.
Domain CHECK constraints and defaults that depend on routines rebuilt from column invalidations are now released and restored around the routine replacement. Domain default drops are routed through the drop phase so pg_depend orders them before routine drops. PostgreSQL 17+ generated columns now rebuild when either side uses a domain type that could reject the temporary NULL reset path.\n\nGREEN evidence: related unit files passed with 79 pass / 0 fail; PG17 focused integration for generated no-cast, aggregate signature replacement, generated domain type, and domain CHECK/default dependency release passed with 4 pass / 0 fail. Static checks passed: format-and-lint:fix, check-types, knip --fix, and git diff --check.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96d7c6c7ae
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
RED evidence before the fix: - generated regular-to-generated rebuild metadata expected true, received false - routine signature column default drop expected 1, received 0 - routine signature domain default drop expected 1, received 0 - routine signature publication table release expected 1, received 0
Release column and domain defaults reached from routine signature replacement roots before dropping the old routine, and release publication table filters when the publication dependency points at a routine rather than a column. Also restore generated-column dependent metadata for regular-to-generated rebuilds where the old column had no default.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e29cf2a889
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
RED evidence before the fix: - does not duplicate existing publication table adds when releasing routine filters failed - audit_accounts ADD count expected 1, received 2
Filter publication release tables per existing ADD/DROP membership changes so expansion only emits missing publication table changes instead of overlapping existing diff output.
1 participant
Summary
Close #263.
This is a follow-up for the rule/trigger cases found during the #263 dependency-replacement audit. It is intentionally separate from #275, which is scoped to RLS policies depending on rewritten columns.
Root cause
pg_dependalready exposes both dependent object kinds:rule:<schema>.<table>.<rule>for rewrite rulestrigger:<schema>.<table>.<trigger>for triggers, includingWHENexpression dependenciesThe gap was in
expandReplaceDependencies: it could walk from a replaced function or an invalidated column to those raw dependents, but it could not resolverule:*/trigger:*stable ids into catalog objects or synthesize replacement changes for them. The original diff therefore leftReplaceRule/ReplaceTriggerchanges asCREATE OR REPLACE ..., which runs too late to unblockDROP FUNCTIONorALTER TABLE ... ALTER COLUMN ... TYPE.Fix
rule:*andtrigger:*dependents from the main and branch catalogs.DROP+CREATEreplacement changes.ReplaceRule/ReplaceTriggerchanges.RED evidence
Before the fix:
expandReplaceDependenciesdid not addDropRulefor dependent rules reached from a procedure signature replacement or an invalidated column.0A000: cannot alter type of a column used by a view or rule.WHEN+ column rewrite failed with PostgreSQL0A000: cannot alter type of a column used in a trigger definition.WHEN+ function signature replacement failed with PostgreSQL2BP01because the old function still had surviving rule/trigger dependents.Validation
On this PR branch:
PGDELTA_SKIP_DUMMY_SECLABEL_BUILD=1 \ PGDELTA_TEST_POSTGRES_VERSIONS=17 \ PGDELTA_CONNECTION_TIMEOUT_MS=15000 \ PGDELTA_CONNECT_TIMEOUT_MS=15000 \ ./node_modules/.bin/bun run --cwd packages/pg-delta test \ --max-concurrency 1 \ --timeout 120000 \ src/core/expand-replace-dependencies.test.ts \ tests/integration/rule-operations.test.ts \ tests/integration/trigger-operations.test.tsResult:
39 pass / 0 fail.Result: passed.
kniponly reported existing configuration hints.I also validated this together with the other local issue-audit branches on a combined branch:
pg-deltafull local run:1624 pass / 0 fail,24 skip,1 todo.pg-topofull test suite:159 pass / 0 fail.Local integration runs used
PGDELTA_SKIP_DUMMY_SECLABEL_BUILD=1, so security-label coverage remains for CI. Testcontainers left by local validation were removed after the run.