Repository navigation
fix(db): rebalance IDENTITY sequences after restore + defensive setval - #2474
Conversation
`pg_dump --clean --if-exists` (used by `genie db backup`) drops every IDENTITY-bearing table and recreates it via DDL — resetting the implicit identity sequence to its initial value (1). The subsequent COPY restores rows with their original explicit ids (1..N), but pg_dump does NOT emit a setval() to advance the sequence past N. The next IDENTITY-generated INSERT then collides with an existing row's PK, breaking every PG-touching CLI command. Observed on khal-os 2026-05-22: `_genie_migrations` had 63 rows but `_genie_migrations_id_seq` was at 48; the first attempt to apply pending migration 064 hit `duplicate key value violates unique constraint "_genie_migrations_pkey"`. Every `genie ls`, `genie task list`, `genie events` invocation failed identically — total CLI lockout until a manual `setval()`. Two-pronged fix: 1. **`rebalanceIdentitySequences()` in db-backup.ts**, called at the end of `restore()`. Walks every `attidentity` column in `public` via a single PL/pgSQL DO block and `setval(seq, GREATEST(MAX(col), 1), is_called)`. Idempotent: empty tables get `setval(1, false)` so the next nextval() yields 1 (canonical IDENTITY-start behavior); non-empty tables advance past MAX(col). Exported so operators on a pre-fix host can recover without re-restoring. Injectable `runner` seam keeps it unit-testable. 2. **Defensive setval in `runMigrations()`** (db-migrations.ts), guarded on `pending.length > 0` so the cost is paid only when actually about to INSERT. Self-heals any host that already drifted via a pre-fix restore, so upgrading to this version is enough — no manual SQL needed. 10 new tests pin the SQL shape, runner contract, error paths (non-zero exit, null status timeout, string-vs-Buffer stderr), and the restore-then-rebalance ordering in `restore()`.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to rebalance PostgreSQL IDENTITY sequences after a database restore to prevent primary key collisions. It includes a new utility to generate and execute PL/pgSQL for sequence alignment, integration into the restore process, and a defensive check within the migration runner. Feedback focuses on optimizing SQL performance by reducing redundant table scans, improving the robustness of sequence name resolution, and refining the type-checking logic for the execution runner to avoid potential runtime errors.
| export function buildIdentityRebalanceSql(): string { | ||
| return `DO $$ | ||
| DECLARE | ||
| r RECORD; | ||
| BEGIN | ||
| FOR r IN | ||
| SELECT n.nspname AS sch, c.relname AS tbl, a.attname AS col | ||
| FROM pg_attribute a | ||
| JOIN pg_class c ON c.oid = a.attrelid | ||
| JOIN pg_namespace n ON n.oid = c.relnamespace | ||
| WHERE a.attidentity IN ('a', 'd') | ||
| AND n.nspname = 'public' | ||
| AND c.relkind = 'r' | ||
| LOOP | ||
| EXECUTE format( | ||
| 'SELECT setval(pg_get_serial_sequence(%L, %L), GREATEST(COALESCE((SELECT MAX(%I) FROM %I.%I), 0), 1), COALESCE((SELECT MAX(%I) FROM %I.%I), 0) > 0)', | ||
| r.sch || '.' || r.tbl, r.col, | ||
| r.col, r.sch, r.tbl, | ||
| r.col, r.sch, r.tbl | ||
| ); | ||
| END LOOP; | ||
| END | ||
| $$;`; | ||
| } |
There was a problem hiding this comment.
The PL/pgSQL block in buildIdentityRebalanceSql performs redundant table scans by executing SELECT MAX(...) twice for every identity column (once for the value and once for the is_called flag). This can be optimized by fetching the maximum value into a variable first. Additionally, using quote_ident when constructing the qualified table name for pg_get_serial_sequence ensures robustness against identifiers that require quoting (e.g., those containing spaces or capital letters).
export function buildIdentityRebalanceSql(): string {
return "DO $$\nDECLARE\n r RECORD;\n m BIGINT;\nBEGIN\n FOR r IN\n SELECT n.nspname AS sch, c.relname AS tbl, a.attname AS col\n FROM pg_attribute a\n JOIN pg_class c ON c.oid = a.attrelid\n JOIN pg_namespace n ON n.oid = c.relnamespace\n WHERE a.attidentity IN ('a', 'd')\n AND n.nspname = 'public'\n AND c.relkind = 'r'\n LOOP\n EXECUTE format('SELECT MAX(%I) FROM %I.%I', r.col, r.sch, r.tbl) INTO m;\n EXECUTE format(\n 'SELECT setval(pg_get_serial_sequence(%L, %L), GREATEST(COALESCE($1, 0), 1), COALESCE($1, 0) > 0)',\n quote_ident(r.sch) || '.' || quote_ident(r.tbl), r.col\n ) USING m;\n END LOOP;\nEND\n$$;";
}| ): void { | ||
| // Accept either an env block (legacy positional) or an options bag, so | ||
| // restore() can keep passing its env and tests can inject a fake runner. | ||
| const isOpts = envOrOpts !== null && typeof envOrOpts === 'object' && 'runner' in envOrOpts; |
There was a problem hiding this comment.
The check 'runner' in envOrOpts is fragile because NodeJS.ProcessEnv is an object that can contain arbitrary keys. If an environment variable named runner exists (e.g., set by a user or CI environment), this logic will incorrectly treat the environment object as an options object, leading to a runtime error when it attempts to call the string value as a function. It is safer to verify that runner is actually a function.
| const isOpts = envOrOpts !== null && typeof envOrOpts === 'object' && 'runner' in envOrOpts; | |
| const isOpts = envOrOpts !== null && typeof envOrOpts === 'object' && typeof (envOrOpts as any).runner === 'function'; |
| await sql.unsafe(`SELECT setval( | ||
| '_genie_migrations_id_seq', | ||
| GREATEST(COALESCE((SELECT MAX(id) FROM _genie_migrations), 0), 1), | ||
| COALESCE((SELECT MAX(id) FROM _genie_migrations), 0) > 0 | ||
| )`); |
There was a problem hiding this comment.
This defensive setval call performs redundant MAX(id) subqueries and relies on a hardcoded sequence name. Using a CTE (Common Table Expression) avoids the double scan, and pg_get_serial_sequence provides a more robust way to resolve the sequence name, ensuring it works even if the sequence naming convention deviates from the default.
await sql.unsafe("\n WITH m AS (SELECT MAX(id) AS val FROM _genie_migrations)\n SELECT setval(\n pg_get_serial_sequence('_genie_migrations', 'id'),\n GREATEST(COALESCE(val, 0), 1),\n COALESCE(val, 0) > 0\n )\n FROM m\n ");
Summary
pg_dump --clean --if-exists(used bygenie db backup→genie db restore) silently desyncs every IDENTITY sequence in the dump. The next IDENTITY-generated INSERT collides with a restored row's PK and breaks every PG-touching CLI command. Observed on khal-os 2026-05-22 (totalgenie lslockout). This PR ships both a corrective fix and a self-healing recovery path.Root cause
pg_dump --clean --if-existsemits DDL that drops + recreates each table. For IDENTITY columns that resets the implicit sequence to its initial value (1). The dump thenCOPYs the rows back in with their original explicit ids (1..N). But pg_dump does not emit a correspondingsetval()to advance the sequence past N. After restore, the sequence is at 1 (or wherever it landed during DDL) while the table holds rows up to N. The next IDENTITY-generated INSERT requests a sequence value (1, 2, 3, …) and collides with an existing PK.This affects any user who ran
genie db backup→genie db restore, on any IDENTITY-bearing table (which is most of them:_genie_migrations,agents,tasks,events,assignments, …).Reproduction
Fix
1.
rebalanceIdentitySequences()insrc/lib/db-backup.ts— called fromrestore()after thepsql ON_ERROR_STOP=1pipe. A single PL/pgSQL DO block walks everyattidentity IN ('a', 'd')column inpublic(skipping views / matviews / foreign / extension tables) and callssetval(pg_get_serial_sequence(...), GREATEST(MAX(col), 1), MAX(col) > 0). Idempotent — empty tables getsetval(1, false)(canonical IDENTITY-start behavior); non-empty tables advance past MAX(col). Exported with an injectablerunnerseam for testability and for operators on a pre-fix host to recover without re-restoring.2. Defensive
setval()inrunMigrations()(src/lib/db-migrations.ts) — guarded onpending.length > 0so the cost is paid only when actually about to INSERT. Self-heals any host that already drifted: upgrading to this version automatically rebalances_genie_migrations_id_seqon first migration apply. No manual SQL needed.Test plan
bun test src/lib/db-backup.test.ts— 16 pass (10 new)bun run typecheck— cleanbun run lint— clean (only pre-existing broken-symlink warnings)genie update --dev+genie lson khal-os and confirm the migration runner self-heals on first invocationFiles touched
src/lib/db-backup.ts(+95) —rebalanceIdentitySequences()+buildIdentityRebalanceSql()+ wiring intorestore()src/lib/db-migrations.ts(+18) — defensivesetval()before the pending-migration INSERT loopsrc/lib/db-backup.test.ts(+108) — 10 regression tests covering SQL shape, runner contract, error paths, restore wiring order