fix: use table recreation for sub_agent_skills unique constraint migration#2208
fix: use table recreation for sub_agent_skills unique constraint migration#2208amikofalvy wants to merge 1 commit intomainfrom
Conversation
…ation Doltgres stores inline UNIQUE constraints as internal keys rather than PostgreSQL-style constraints, making them impossible to drop via standard DROP CONSTRAINT or DROP INDEX syntax. Replace the DROP/ADD CONSTRAINT approach with a full table recreation to work around this Doltgres limitation. Co-authored-by: Cursor <cursoragent@cursor.com>
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: High
🔴❗ Critical (1) ❗🔴
Inline Comments:
- 🔴 Critical:
0010_oval_angel.sql:1-20Editing an already-applied migration file violates AGENTS.md migration workflow
✅ What's Done Well
The table recreation pattern itself is the correct approach for working around Doltgres's inability to drop inline UNIQUE constraints. The implementation correctly:
- Creates a new table with the expanded unique constraint scope
- Preserves all data via
INSERT INTO ... SELECT * FROM - Recreates foreign keys and indexes after the rename
- Matches the Drizzle schema definition in
manage-schema.ts
💭 Consider
💭 1) scope Add a comment documenting the Doltgres limitation
Issue: Future maintainers may not understand why the table recreation pattern was used instead of standard ALTER TABLE constraint modification.
Why: The PR description documents this well, but that context will be lost once the PR is merged. A SQL comment in the migration file explaining the Doltgres limitation would help future maintainers.
Fix: Add a comment at the top of the migration:
-- Doltgres stores inline UNIQUE constraints as internal keys rather than
-- PostgreSQL-style constraints, making them impossible to drop via standard
-- syntax. This migration uses table recreation as a workaround.🚫 REQUEST CHANGES
Summary: The table recreation approach is technically correct for the Doltgres workaround, but this PR edits an existing migration file that already exists on main. Per AGENTS.md, existing migrations should never be modified after being applied — instead, create a new migration file (e.g., 0011_fix_sub_agent_skills_constraint.sql) that handles the fix. This ensures databases that already ran migration 0010 will get the correct schema state via the new migration.
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
meta/0010_snapshot.json |
Snapshot should reflect constraint columns | Not an issue — the snapshot correctly shows the expanded constraint columns and was not modified by this PR |
scope |
Migration should be tested against PGlite | Not applicable — PGlite is used for tests but the fix is specifically for Doltgres behavior |
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-breaking-changes |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
pr-review-devops |
3 | 0 | 0 | 0 | 0 | 0 | 2 |
| Total | 4 | 0 | 0 | 0 | 1 | 0 | 2 |
Note: Both reviewers identified the same critical issue (editing existing migration), consolidated into single inline comment.
| CREATE TABLE "sub_agent_skills_new" ( | ||
| "tenant_id" varchar(256) NOT NULL, | ||
| "id" varchar(256) NOT NULL, | ||
| "project_id" varchar(256) NOT NULL, | ||
| "agent_id" varchar(256) NOT NULL, | ||
| "sub_agent_id" varchar(256) NOT NULL, | ||
| "skill_id" varchar(64) NOT NULL, | ||
| "index" numeric DEFAULT 0 NOT NULL, | ||
| "always_loaded" boolean DEFAULT false NOT NULL, | ||
| "created_at" timestamp DEFAULT now() NOT NULL, | ||
| "updated_at" timestamp DEFAULT now() NOT NULL, | ||
| CONSTRAINT "sub_agent_skills_tenant_id_project_id_agent_id_id_pk" PRIMARY KEY("tenant_id","project_id","agent_id","id"), | ||
| CONSTRAINT "sub_agent_skills_sub_agent_skill_unique" UNIQUE("tenant_id","project_id","agent_id","sub_agent_id","skill_id") | ||
| );--> statement-breakpoint | ||
| INSERT INTO "sub_agent_skills_new" SELECT * FROM "sub_agent_skills";--> statement-breakpoint | ||
| DROP TABLE "sub_agent_skills";--> statement-breakpoint | ||
| ALTER TABLE "sub_agent_skills_new" RENAME TO "sub_agent_skills";--> statement-breakpoint | ||
| ALTER TABLE "sub_agent_skills" ADD CONSTRAINT "sub_agent_skills_sub_agent_fk" FOREIGN KEY ("tenant_id","project_id","agent_id","sub_agent_id") REFERENCES "public"."sub_agents"("tenant_id","project_id","agent_id","id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "sub_agent_skills" ADD CONSTRAINT "sub_agent_skills_skill_fk" FOREIGN KEY ("tenant_id","project_id","skill_id") REFERENCES "public"."skills"("tenant_id","project_id","id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| CREATE INDEX "sub_agent_skills_skill_idx" ON "sub_agent_skills" USING btree ("skill_id"); |
There was a problem hiding this comment.
🔴 CRITICAL: Editing an already-applied migration file
Issue: This PR modifies 0010_oval_angel.sql, an existing migration file that already exists on main (introduced in PR #2190, reverted in #2205, re-reverted in #2207). Per AGENTS.md line 147: "NEVER edit existing migration SQL files after they've been applied — create new migrations instead."
Why: Drizzle tracks applied migrations by filename in drizzle.__drizzle_migrations. When the migration file content changes but the filename stays the same:
- Databases where 0010 already ran will not re-run the new SQL (migration marked as "applied")
- Those databases will have inconsistent schema state (either the old constraint columns if it succeeded, or a broken state if the original Doltgres limitation caused it to fail)
- New databases will get the correct table recreation approach, causing schema drift between environments
Fix: Create a new migration file (e.g., 0011_fix_sub_agent_skills_constraint.sql) that:
- Applies the table recreation pattern for databases that need it
- Is idempotent (safe to run on databases in any state)
Example approach:
-- 0011_fix_sub_agent_skills_constraint.sql
-- Check if we need to apply the fix (constraint doesn't have all 5 columns)
-- Then apply table recreation patternAlternatively, if you're certain migration 0010 has never been successfully applied to any environment (failed everywhere due to Doltgres limitation), document that assumption clearly and consider whether the rollback strategy is acceptable.
Refs:
…traint migration The previous migration used DROP CONSTRAINT + ADD CONSTRAINT which fails on Doltgres (can't drop UNIQUE constraints). PR #2208's approach of creating a _new table with same constraint names caused PGlite index name collisions. This uses backup→drop→recreate→restore: copy data to a temp table, drop the original (freeing all names), recreate with the wider UNIQUE constraint, then restore data. Tested on both Doltgres v0.55.3 and PGlite with zero data loss. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Closing in favor of #2214 |
…traint migration The previous migration used DROP CONSTRAINT + ADD CONSTRAINT which fails on Doltgres (can't drop UNIQUE constraints). PR #2208's approach of creating a _new table with same constraint names caused PGlite index name collisions. This uses backup→drop→recreate→restore: copy data to a temp table, drop the original (freeing all names), recreate with the wider UNIQUE constraint, then restore data. Tested on both Doltgres v0.55.3 and PGlite with zero data loss. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
pnpm db:migratefailure caused by Doltgres's inability to drop inline UNIQUE constraints via standard PostgreSQL syntaxDROP CONSTRAINT/ADD CONSTRAINTapproach in migration0010_oval_angel.sqlwith a full table recreation (create new table → copy data → drop old → rename) to work around the Doltgres limitationsub_agent_skillsis widened from(sub_agent_id, skill_id)to(tenant_id, project_id, agent_id, sub_agent_id, skill_id)to properly scope it to tenant/project/agentContext
Doltgres stores inline
UNIQUEconstraints fromCREATE TABLEas internal keys rather than PostgreSQL-style constraints. This makes them impossible to drop through any standard syntax:ALTER TABLE ... DROP CONSTRAINTALTER TABLE ... DROP CONSTRAINT IF EXISTSADD CONSTRAINTfails with "Duplicate key name"DROP INDEX IF EXISTS(standalone)ALTER TABLE ... DROP INDEX(MySQL syntax)The table recreation approach sidesteps this entirely by creating a fresh table with the correct constraint.
Test plan
pnpm db:migratelocally and verify it completes successfullysub_agent_skillstable has the correct unique constraint after migrationpnpm checkto ensure no regressionsMade with Cursor