Repository navigation
docs: explain why --desired and --alter treat a destructive drop differently - #268
Conversation
…erently The constraint-rename row said running the statements with --alter converges, which predates --allow-destructive; the drop now needs the acknowledgement.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
🤖 Review on Jash's behalf at Verdict: the documentation clarifies the two acknowledgement paths; no blocking defect established in the inspected diff. The table now includes the table-scoped destructive acknowledgement, and the new subsection consistently distinguishes inferred desired-state drops from explicitly supplied ALTERs.
Verified: read the description, full one-file diff and available issue comments; checked the new TOC/link anchor against the heading; no runtime code or tests were changed or removed. Six CI checks succeeded, including unit tests/docs guards; six runtime/build/compatibility jobs were skipped. The separate automated Codex review failed due to usage limits, not a reported code finding. Exact-head AGENTS, full surrounding documentation/source and inline-comment API reads failed connecting to api.github.com, so the current CLI implementation was not independently verified. No local build or tests ran. Generated by Codex. |
|
🤖 Product review at OSS adoption: Naming SchemaBot integration: This changes documentation only, with no library API delta. The useful distinction is that a desired-plan refusal is not permission to replay its generated DROP through an acknowledged ALTER automatically; that requires a separate explicit operator decision. The CLI/library enforcement of that boundary was not independently verified here because surrounding source was unavailable. Generated by Codex. |
JashLal
left a comment
There was a problem hiding this comment.
🤖 Approving on Jash’s behalf after the review at 1657fc1. No blocking defect established in the documentation diff; the optional UNIQUE enforcement-gap caveat is recorded in the review. Exact-head surrounding source and inline threads were unavailable, and no local tests ran. Approval does not certify those unverified paths. Generated by Codex.
Dropping first leaves the column without a unique constraint until the add commits. The example now gives the commands in add, drop, desired order, says what to do if the add fails, and that a refused desired plan is no licence to run its statements unattended.
|
🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/268, follow-up commit All findings are fixed in the docs. The example now runs the
Source: block/pg-sprite#268, review comments 6458475946 and 6458478378 and review 5676797748 at head |
Explains in
docs/execute-and-introspect.mdwhymigrate --desiredrefuses a destructive plan whole, whilemigrate --alterruns the same drop once--allow-destructivenames the table. It also corrects the constraint-rename row, which left out the flag, and gives the commands in an order that keeps the column's uniqueness enforced throughout.Why
The "What the catalog does not fold" table said a constraint rename "converges" by running the statements with
migrate --alter. Since--allow-destructivelanded, that drop is refused withdestructive-changeunless the flag names the table, so following the row as written ends in a refusal. The row also gave no reason why the two front doors behave differently, which reads as an inconsistency. Running the drop before the add, as the plan lists them, would also leaveemailwithout a unique constraint until the add commits.What
migrate --alter, then the drop withmigrate --alter --allow-destructive public.accounts, thenmigrate --desiredfor the rest, and links to the new subsection.--desiredand--altertreat the drop differently":--desired, pg-sprite derived the drop from a diff and cannot tell a rename from removing a guarantee, so it refuses the whole plan with no override.--alter, the operator wrote the drop, and--allow-destructiveacknowledges the loss for that table.--alterstatements on its own.The sequence was run against PostgreSQL 16 with main's build:
--desiredrefuses withdestructive-change, and the add runs as the saferCREATE UNIQUE INDEX CONCURRENTLYsequence.23505.--allow-destructive public.accountsis given.--desiredthen runs the twoSET DEFAULTs, anddiffreports no changes.Before / after
The desired file renames the live unnamed
UNIQUE (email)toCONSTRAINT accounts_email_uniq UNIQUE; the diff plansDROP CONSTRAINT accounts_email_key+ADD CONSTRAINT accounts_email_uniq.🤖 Generated with Amp (Claude Opus 4.6)