Skip to content

Fix prefixed migrations: schema-qualified index names + cross-schema existence checks - #628

Merged
ddon merged 3 commits into
BeamLabEU:mainfrom
mdon:main
Jul 11, 2026
Merged

Fix prefixed migrations: schema-qualified index names + cross-schema existence checks#628
ddon merged 3 commits into
BeamLabEU:mainfrom
mdon:main

Conversation

@mdon

@mdonmdon commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Summary

An external user running the full migration chain into a named schema (--prefix "companyplexus") hit a hard failure. Investigating it surfaced two bug families in the versioned migrations, both invisible on the default public path:

1. Schema-qualified index names on CREATE INDEX (hard failure). Postgres rejects CREATE INDEX schema.name ... outright — an index always lands in its table's schema, so only the table may be qualified. (DROP INDEX schema.nameis valid, which is why the drop helpers using the same idiom never failed — those are untouched.) With a non-public prefix the chain died with syntax error at or near ".". Five sites fixed: ensure_fk_target_unique_indexes/2 and create_uuid_fk_index/4 in uuid_fk_columns.ex, add_uuid_unique_indexes in V56 and V57, and the V95 media-folders unique index. Note these errors surface at a later version's flush() (V61), not the version that queued the SQL.

2. Cross-schema-leaky idempotency checks (silent skips). Existence checks that don't anchor to a schema see objects from every schema. When a prefixed install runs in a database that also carries a public install, the check finds public's object and silently skips creating the prefixed one — the prefixed schema ends up missing columns/constraints (or hard-fails downstream, which is how the new test caught it). Fixed: V95's information_schema.columns check gained a table_schema filter, and 13 pg_constraintconname checks gained the AND conrelid = '<prefixed table>'::regclass anchor (V51's existing idiom): V35, V102 ×5, V113 ×2, V115, V118 up+down, V119 up+down. The other 43 pg_constraint sites were already anchored.

3. Regression test.test/integration/prefix_migration_test.exs runs the full 142-version chain into a scratch schema on the test database and asserts the version marker, the once-buggy indexes' placement, and a full index complement. Because the test DB also carries a public install, it fails on either bug family. It flips the sandbox to :auto for the run (the same boot path test_helper.exs uses) — a sandbox checkout can't host Ecto.Migrator's own runner connections, and a dynamic non-sandbox repo instance doesn't work either because V08's backfill resolves the repo by module name (Ecto.Adapters.SQL.query(RepoHelper.repo(), ...)), bypassing put_dynamic_repo/1. Details in the test moduledoc.

Verification

  • Fresh scratch DB, prefix: "companyplexus": chain previously died at V61; now completes with all 730 phoenix_kit indexes in the prefixed schema, none leaked to public
  • Fresh scratch DB, default public: completes (no regression)
  • Prefixed chain into a DB with an existing public install (the new test): passes in ~5s
  • Permissions/migration/smoke test files: 159 tests, 0 failures
  • mix precommit (format + compile + credo --strict + dialyzer): green
  • Reviewed by Codex: confirmed no missed sites of either family, ::regclass casts safe (every target table is guaranteed to exist at its check), and prefixed-vs-public semantics correct; its one finding (test cleanup could leave the sandbox in :auto if the schema drop raises) is fixed with try/after

mdon added 3 commits July 11, 2026 04:17
Postgres rejects CREATE INDEX schema.name outright - an index always
lands in its table's schema, so only the table may be qualified
(DROP INDEX schema.name is valid; those sites are untouched). With
--prefix set to a non-public schema the whole migration chain died
with a syntax error. Reported by an external user running the full
chain with --prefix "companyplexus".
Five sites fixed: ensure_fk_target_unique_indexes/2 and
create_uuid_fk_index/4 in uuid_fk_columns.ex, add_uuid_unique_indexes
in V56 and V57, and the V95 media-folders unique index.
Idempotency checks that match on conname alone see constraints from
every schema in the database. When a prefixed install runs in a
database that also carries a public install, the check finds public's
constraint and silently skips creating it on the prefixed table -
leaving the prefixed schema without its CHECK/FK constraints.
Add the AND conrelid = '<prefixed table>'::regclass anchor (V51's
existing idiom) to the 13 unanchored sites: V35, V102 (5), V113 (2),
V115, and V118/V119 (up and down each). The V95 information_schema
column check gained its table_schema filter in the previous commit;
the other 43 pg_constraint sites were already anchored.
Nothing exercised the --prefix path before: every fleet app and the
test boot migrate into public, so the qualification branches never
ran. The test runs the full 142-version chain into a scratch schema
on the test database, asserts the version marker reports the chain
fully applied, spot-checks the indexes built by the once-buggy CREATE
sites, and requires a full index complement in the prefixed schema.
Because the test database also carries a public install, it covers
the cross-schema existence-check family too - it fails on either bug
class.
The chain runs through the named repo with the sandbox flipped to
:auto (the boot path test_helper uses), restored to :manual in
on_exit even if cleanup raises. A sandbox checkout cannot host the
migrator (it spawns its own runner with its own connections), and a
second non-sandbox repo instance does not work either: V08's backfill
calls Ecto.Adapters.SQL.query(RepoHelper.repo(), ...), which resolves
the repo module name to the named instance and bypasses
put_dynamic_repo/1.
@ddon
ddon merged commit dc4f7c7 into BeamLabEU:mainJul 11, 2026
ddon pushed a commit that referenced this pull request Jul 11, 2026
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ddon pushed a commit that referenced this pull request Aug 9, 2026
Root cause (empirically proven on the scratch DB): execute/1 only QUEUES DDL
while the chain's existence guards are immediate repo().query reads — v56
called UUIDFKColumns.add_constraints with no flush() after UUIDFKColumns.up,
and v57 had no flush() at all, so single-shot installs (fresh projects) never
generated SET NOT NULL for 46 *_uuid columns and never created the comments
FK at V56/57; v72 then filled the 'missing' FK with a guessed ON DELETE
CASCADE while every incrementally-upgraded install carries V57's SET NULL.
Fix: flush() discipline in v56/v57 (in-place hardening precedent PR #628/631);
v72's comments entry aligned to the intended SET NULL (content anonymizes on
user deletion — likes/dislikes stay CASCADE); new V161 repairs
already-affected single-shot installs: SET NOT NULL only where zero NULL rows
exist (warn+skip otherwise — never backfills live data), comments FK corrected
metadata-only via name-anchored pg_constraint check. UUIDFKColumns gains the
not_null_uuid_fks/0 accessor so V161 shares the canonical list.
Verified: both modes now agree to V161 on all pairs + the FK; full generator
run completes with NO shape mismatch and emits the first complete
ExpectedSchema manifest (159 tables / 1869 columns / 417 constraints / 609
indexes / 114 seeds). Known follow-ups tracked for P3: prefix-embedded index
NAMES still enter the manifest untemplated (12 uuid-unique indexes,
:legacy_optional misclass), and UUIDFKColumns carries a dead symmetric
subscriptions.plan_uuid entry.
Sign up for freeto 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

@mdon@ddon