Skip to content

docs: explain why --desired and --alter treat a destructive drop differently - #268

Merged
Kiran01bm merged 2 commits into
mainfrom
kiran01bm/docs-destructive-front-doors
Oct 28, 2026
Merged

Kiran01bm merged 2 commits into
mainfrom
kiran01bm/docs-destructive-front-doors

Conversation

@Kiran01bm

@Kiran01bm Kiran01bm commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Explains in docs/execute-and-introspect.md why migrate --desired refuses a destructive plan whole, while migrate --alter runs the same drop once --allow-destructive names 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-destructive landed, that drop is refused with destructive-change unless 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 leave email without a unique constraint until the add commits.

What

  • The constraint-rename row now says to run the add with migrate --alter, then the drop with migrate --alter --allow-destructive public.accounts, then migrate --desired for the rest, and links to the new subsection.
  • New subsection "Why --desired and --alter treat the drop differently":
    • Under --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.
    • Under --alter, the operator wrote the drop, and --allow-destructive acknowledges the loss for that table.
    • It gives the three copyable commands, add before drop, and says why that order matters: both constraints enforce uniqueness while both stand. It also says what happens if the add fails: the old constraint still enforces uniqueness, and the invalid-index runbook covers a failed concurrent build.
    • A refused desired plan is not permission for an orchestrator to run its statements as acknowledged --alter statements on its own.

The sequence was run against PostgreSQL 16 with main's build:

  • --desired refuses with destructive-change, and the add runs as the safer CREATE UNIQUE INDEX CONCURRENTLY sequence.
  • While both constraints stand, a duplicate insert is refused with 23505.
  • The drop is refused, then commits once --allow-destructive public.accounts is given.
  • --desired then runs the two SET DEFAULTs, and diff reports no changes.

Before / after

The desired file renames the live unnamed UNIQUE (email) to CONSTRAINT accounts_email_uniq UNIQUE; the diff plans DROP CONSTRAINT accounts_email_key + ADD CONSTRAINT accounts_email_uniq.

Before (doc)                                  After (doc)

migrate --desired → whole plan refused         migrate --desired → whole plan refused,
  (destructive-change), no reason given          because pg-sprite derived the drop and
                                                 cannot tell a rename from a removal
"running the statements with                   1. migrate --alter 'ADD CONSTRAINT …' → runs;
 migrate --alter converges"                       email now has two unique constraints
  → operator runs the DROP with --alter        2. migrate --alter 'DROP CONSTRAINT …'
  → refused: destructive-change                   --allow-destructive public.accounts → runs;
    (the doc did not mention the flag)            email was never unenforced
  → with the flag, DROP then ADD leaves        3. migrate --desired → the SET DEFAULTs run,
    email unenforced in between                   fresh plan empty: converged

🤖 Generated with Amp (Claude Opus 4.6)

…erently

The constraint-rename row said running the statements with --alter converges,
which predates --allow-destructive; the drop now needs the acknowledgement.
@Kiran01bm
Kiran01bm marked this pull request as ready for review October 28, 2026 01:04
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ⚠️ Failed 2026-10-28T01:04:24.006418Z
We couldn’t complete this review. Comment @codex review to try again.
1657fc1 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@JashLal

JashLal commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator

🤖 Review on Jash's behalf at 1657fc18c80a9decd8305b5715990b0733720ec8.

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.

  1. (Optional, docs/execute-and-introspect.md:237–245) Add a short caveat beside the drop-then-add UNIQUE example: between the two operations, uniqueness is unenforced. Concurrent writes can introduce duplicates, so the ADD may fail and leave the constraint absent. Clarify what “the safer UNIQUE sequence” means and what the operator should do if ADD fails. This is an operational guidance improvement, not a demonstrated regression in this docs-only change.

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.

@JashLal

JashLal commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator

🤖 Product review at 1657fc18c80a9decd8305b5715990b0733720ec8.

OSS adoption: Naming public.accounts in the acknowledgement and explaining the whole-plan refusal should reduce trial and error. Optional: make the example fully copyable, including the quoted --alter SQL, and link the uniqueness-gap/recovery caveat from the correctness review.

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 JashLal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.
@Kiran01bm

Copy link
Copy Markdown
Collaborator Author

🤖 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 ADD before the DROP, so email is never unenforced, and the full sequence was run against PostgreSQL 16 with main's build.

# Finding Status Explanation
C1-F1 Drop-then-add leaves uniqueness unenforced in between; say what "the safer UNIQUE sequence" is and what to do if the ADD fails fixed The example now runs the ADD first, then the DROP, then migrate --desired for the rest. That closes the gap: while both constraints stand, a duplicate insert is refused with 23505, verified on a real server. The doc names the safer sequence (CREATE UNIQUE INDEX CONCURRENTLY, then ADD CONSTRAINT … UNIQUE USING INDEX). It also says that if the ADD fails, the old constraint still enforces uniqueness, and links the invalid-index runbook for a failed concurrent build.
C2-F1 Make the example fully copyable, including the quoted --alter SQL fixed The subsection gives the three commands in order. It also corrects the old "rerun diff → converged" line: after the two --alter statements, diff still lists the two SET DEFAULTs, so migrate --desired finishes the example.
C2-F2 A refused desired plan is not permission to replay its drop through an acknowledged --alter automatically fixed The subsection now says so: an orchestrator should not turn a refused plan into acknowledged --alter statements on its own; running the drop is a separate operator decision. --allow-destructive with --desired is already rejected by the CLI.
Review Approval no action —

Source: block/pg-sprite#268, review comments 6458475946 and 6458478378 and review 5676797748 at head 1657fc18; fixes in the follow-up commit.

@Kiran01bm
Kiran01bm enabled auto-merge (squash) October 28, 2026 05:15
@Kiran01bm
Kiran01bm merged commit df229a2 into main Oct 28, 2026
12 checks passed
@Kiran01bm
Kiran01bm deleted the kiran01bm/docs-destructive-front-doors branch October 28, 2026 05:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants