Uh oh!
There was an error while loading. Please reload this page.
Add V163: repair uuid columns that are not primary keys - #688
Merged
Conversation
A production database reached a state the chain is supposed to make impossible: phoenix_kit_email_events with uuid as character varying(255), nullable, no default, and NO primary key, while 149 other tables were correct. Three migrations each missed it for a different reason. V40's guard tests column EXISTENCE, not TYPE. An older release created the column as Ecto :string, so `unless column_exists?(table, :uuid, …)` was already true and V40 skipped the table wholesale — not just the ADD COLUMN but the backfill, the SET NOT NULL and the unique index. The table IS in V40's @tables_to_migrate; being listed did not help. V56 would have converted it — `ensure_all_uuid_columns_native_type/2` does exactly that — but it was added to V56 on 2026-03-02, seventeen days after V56 shipped on 2026-02-13. A recorded version never re-runs, so every host that crossed V56 in that window kept the broken column permanently. V56's NOT NULL and index repairs also run off hardcoded lists this table appears in none of. V74 then dropped the legacy bigint id but could not promote uuid to primary key — wrong type, nullable — and did not verify its documented post-condition. So V163 is catalog-driven. Every previous attempt enumerated tables by hand and this one was missing from every list, including the list in the migration written to repair its class of problem. It asks the catalog what is actually broken instead. Large tables are deferred rather than silently rewritten: ALTER COLUMN TYPE rewrites under an ACCESS EXCLUSIVE lock and ADD PRIMARY KEY builds an index under the same one, both O(rows). On a big events table behind PgBouncer that is pool exhaustion during migrate, not a pause. Above two million rows the repair is skipped and logged with the command to run in a maintenance window; mix phoenix_kit.repair_uuid is that command, and builds the index CONCURRENTLY because it runs outside a transaction. Nothing raises on the happy path. The generated upgrade migration carries @disable_ddl_transaction, so each table is isolated: a lock_timeout makes a contended table fail fast instead of hanging a deploy, a rescue keeps one bad table from taking the rest of the run with it, and the version marker is written regardless. A skipped table stays visible in the doctor; an unwritten marker would be far worse, and is how this codebase has previously had migrations skipped permanently. The doctor gains a primary-key check — the type check could not see a missing key, which is what the varchar column actually cost — and its remedy is corrected: the single ALTER it suggested restores the type but not the NOT NULL, the default or the key, leaving anyone who followed it literally still broken.
`Ecto.Migration.execute/1` QUEUES a command rather than running it, so the statements were flushed after `up/1` returned — outside the `rescue` that exists to keep one table's failure from aborting the rest of the run. The isolation compiled, read correctly, and would have done nothing: a locked table would still have taken down every repair after it AND skipped the version marker, which is the worst outcome available here. `flush/0` runs the pending commands in scope, so the rescue actually catches. Caught by a review pass that was asked specifically whether `execute/1` is deferred; it is exactly the kind of defect that looks fine in a diff. Also derives the concurrent index name from the catalog name instead of parsing it back out of the quoted qualified string, which would corrupt any name containing a dot or a quote.
ddon pushed a commit
that referenced
this pull request
Aug 9, 2026
Upstream merged its own V163 (uuid primary-key integrity, PR #688) while this branch was in flight, so the repair delta this branch carries moves to V164 to leave that number to it. Their repair runs first by construction, which is the order this one needs: it promotes uuid columns to primary keys, and a foreign key cannot reference a column that has none. Also folds in what a draft carried as a separate V164 — this release ships one migration, not two — and closes the GLM/Opus review round: shape-based FK matching, a guarded ADD CONSTRAINT, SQLSTATE in the validate handler, an end-of-run NOT VALID summary, a bounded advisory-lock wait, plus the mode-A public-path oracle and the s21/s22 scenarios that gate all of it.
ddon pushed a commit
that referenced
this pull request
Aug 9, 2026
Post-merge review of the migration work (#688/#689 plus the two fix commits), by Kimi K3 at max effort. Three of its findings, all verified against the tree. `Common.query_version_directly/2` ended with `# Table exists but no version comment - assume version 1`, so a database that is current but lost its comment reported `{:current_version, 1}` — below the floor — and `phoenix_kit.update` told the operator to install the 1.7.x bridge. `Postgres.migrated_version/1` refuses that same state, in its own words: the guess "would route a possibly CURRENT database to the 1.7.x bridge, whose backfill overwrites still-NULL tracked columns with freshly generated uuids pointing at nothing". On the bridge, `UUIDFKColumns.set_not_null/4` invents uuids for legitimately-NULL `phoenix_kit_files` rows and `cleanup_orphaned_fk_refs/5` then deletes them for matching no user. So two halves of one release gave opposite instructions for one state, and the destructive one was the one the operator met first. The sibling fallback in the same module had already been hardened against the same guess — its comment records that it "used to fall back to a fabricated {:current_version, 1}… Report honestly instead" — but the fabrication survived one level down. `:unknown_version` is now distinct from every real version and from "not installed", and the update task routes it to doctor + restamp, matching what the migrator says. Two smaller findings from the same round: - `Repair.Probe.read_comment/2` parsed the comment with `String.to_integer/1` eleven lines below a docstring promising it never raises, so the exact hand-edits the migrator documents (`'v164'`, `' 164'`) ended `mix phoenix_kit.repair` in a bare `** (ArgumentError) argument error`. The tool an operator reaches for when the comment is already anomalous now treats an unparseable one as "no usable version", which its callers already handle. The same unguarded parse in `Common` is fixed by the change above. - The below-floor notice said "the last PhoenixKit 1.7.x release". The version was already threaded into both `BelowFloorError` raise sites but not into the notice shown FIRST, at generation time — the raises are only reached later. `Postgres.bridge_version/0` exposes it so both name the same release. compile --warnings-as-errors, format --check-formatted and credo --strict (10290 mods/funs) clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a reported production defect:
phoenix_kit_email_eventswithuuidascharacter varying(255), nullable, no default, and no primary key, on a database upgraded through the whole chain since V01. 149 other tables were correct, so this was one table falling out of the conversion rather than a broken upgrade.Why the chain missed it — three times
The reporter identified V40's guard. There is a second cause they could not see from outside the repo, and it is the one that explains why they are still broken at V162.
1. V40's guard tests EXISTENCE, not TYPE. An older release created the column as Ecto
:string, sounless column_exists?(table, :uuid, …)was already true and V40 skipped the table wholesale — not just theADD COLUMN, but the backfill, theSET NOT NULLand the unique index with it. The table is in V40's@tables_to_migrate; being listed did not help.2. V56's type conversion shipped 17 days after V56 did.
ensure_all_uuid_columns_native_type/2converts exactly this varchar column — and it was added to V56 on 2026-03-02, while V56 itself shipped 2026-02-13. A recorded version never re-runs, so every host that crossed V56 in that window kept the broken column permanently. V56'sNOT NULLand index repairs also run off hardcoded table lists thatphoenix_kit_email_eventsappears in none of.3. V74 did not verify its own post-condition. It dropped the legacy bigint
idbut could not promoteuuidto primary key — wrong type, nullable — and its moduledoc's claim ("after V74, every PhoenixKit table has uuid as its PK") silently became false.What this adds
V163 — catalog-driven repair. Every previous attempt enumerated tables by hand and this table was absent from every list, including the list in the migration written to repair its class of problem. V163 asks the catalog which tables are actually wrong, so a table nobody remembered to list is repaired anyway.
PhoenixKit.Migrations.UUIDIntegrity— the detection and SQL, shared by the migration and the task so they cannot drift. Drift in a primary-key repair is how a table ends up half-fixed.mix phoenix_kit.repair_uuid— the operator path for tables too large to rewrite during a deploy. Supports--dry-run,--prefix, and specific table names, and builds the unique indexCONCURRENTLYbecause it runs outside a transaction.Doctor — a new primary-key check, plus a correction to the existing one.
Deliberate decisions
Large tables are deferred, not silently rewritten.
ALTER COLUMN TYPErewrites underACCESS EXCLUSIVEandADD PRIMARY KEYbuilds an index under the same lock — both O(rows). On a big events table behind PgBouncer (which the reporter runs) that is pool exhaustion duringmix ecto.migrate, not a pause. Above two million rows the repair is skipped and logged with the exact command; the doctor keeps reporting it until it is done.Nothing raises on the happy path. The reporter suggested V74 verify and fail. Raising on an already-shipped chain would strand every affected host mid-deploy, and a library does not own its hosts' deploy runbooks. Turning "one audit table lacks a PK, on a version where no schema maps to it" into a fleet-wide failed deploy is the worse outcome. The doctor is the loud channel.
The version marker is written even when a table is skipped. An unwritten marker is how this codebase has previously had migrations skipped permanently. A skipped table stays visible; a stale marker does not.
The doctor's remedy was incomplete and is corrected. It suggested
ALTER TABLE <t> ALTER COLUMN uuid TYPE uuid USING uuid::uuid, which restores the type but not theNOT NULL, the UUIDv7 default, or the primary key — anyone following it literally was left in a state that still fails the check. It now points at the task. The "will crash Ecto on load" wording is also softened: on 1.7.235 no schema maps to this table, so it overstated the impact, exactly as the reporter noted.Review found two defects in this change
Both were caught by an adversarial pass over the diff and are fixed here:
Ecto.Migration.execute/1queues a command, so the statements were flushed afterup/1returned — outside therescue. A locked table would still have aborted every later repair and skipped the marker.flush/0now runs them in scope. This compiled and read correctly while doing nothing, which is the kind of defect a diff hides.quote_ident/1now quotes both prefix and table; there is a test with a space in the name.Verification
mix precommitexit 0V162 → V163, doctor's new check PASS.::regclassanywhere (it raises when the relation is absent and aborts the transaction), every existence check anchored ontable_schema/nspname,Helpers.uuid_v7_call/1for the generator.No version bump or CHANGELOG entry — left at the current version deliberately.
One thing I could not attribute: an intermittent single failure in
test/integration/phoenix_kit_web/live/users/media_test.exs("orphan filter false omits orphaned param"). It passes in isolation on both a clean tree and this branch, appears seed-dependent, and is unrelated to migrations — but it did not appear on the one clean-tree full run I did, so I am flagging it rather than asserting it is pre-existing.