feat(sync_jobs): validate id_big NOT NULL (NAN-5491 Phase 3d) - #6379
Merged
Conversation
Lowers CRON_DELETE_OLD_JOBS_MAX_DAYS default from 31 to 3 ahead of the sync_jobs id widening. The cron's daily prune brings _nango_sync_jobs down to ~3 days of rows, sizing the table down before any expensive DDL or backfill in subsequent phases. To be reverted in Phase 3g once the widening is complete. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…Phase 3b) ALTER TABLE adds a nullable bigint id_big alongside the int4 id. A BEFORE INSERT OR UPDATE row trigger sets NEW.id_big := NEW.id so every new write populates id_big automatically — no per-call-site code change needed. Phase 3c backfills any rows that existed before the trigger. Phase 3e performs the atomic PK swap. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
UPDATE fills any leftover NULL id_big rows. After Phase 3a's 3-day retention wait, the trigger from 3b has populated id_big on every row written post-trigger and retention dropped the rest, so on cloud this matches ~0 rows. Cloud production: run by hand (verify EXISTS NULL = f first) and INSERT the migrations row to skip auto-run — avoids the scan cost during persist/server startup. Self-hosted has a tiny table so the auto-run is sub-second. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Migration body uses non-CONCURRENTLY CREATE UNIQUE INDEX IF NOT EXISTS — safe for self-hosted's tiny _nango_sync_jobs (sub-second build under ACCESS EXCLUSIVE). Cloud production: run CREATE UNIQUE INDEX CONCURRENTLY sync_jobs_id_big_uidx by hand (no IF NOT EXISTS so an invalid leftover doesn't silently no-op), verify indisvalid = t, then INSERT INTO _nango_auth_migrations to skip the auto-run. The unique index is the constraint backbone of Phase 3e's atomic PK swap (ADD CONSTRAINT … PRIMARY KEY USING INDEX). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a NOT VALID CHECK constraint proving id_big has no NULL rows, then VALIDATEs it. The validation scans the table under SHARE UPDATE EXCLUSIVE so concurrent reads and writes keep flowing. The validated CHECK is what lets Phase 3e's swap (SET NOT NULL on id, ADD PRIMARY KEY USING INDEX) stay metadata-only under ACCESS EXCLUSIVE instead of scanning the table while holding the table lock. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
Contributor
There was a problem hiding this comment.
1 issue found across 1 file
Confidence score: 3/5
packages/database/lib/migrations/20260604120250_sync_jobs_validate_id_big_not_null.cjscombinesADD CONSTRAINT ... NOT VALIDwithVALIDATE CONSTRAINTin one transaction, which can keep a stronger lock until commit and block concurrent writes.- This is a concrete runtime risk (severity 7/10, confidence 8/10) that can cause user-facing write disruption during deployment, so merge risk is moderate rather than low.
- Pay close attention to
packages/database/lib/migrations/20260604120250_sync_jobs_validate_id_big_not_null.cjs- split/sequence validation to reduce lock duration and avoid write blocking.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/database/lib/migrations/20260604120250_sync_jobs_validate_id_big_not_null.cjs">
<violation number="1" location="packages/database/lib/migrations/20260604120250_sync_jobs_validate_id_big_not_null.cjs:4">
P1: This migration runs `ADD CONSTRAINT ... NOT VALID` and `VALIDATE CONSTRAINT` in the same transaction, which can hold the stronger lock until commit and block writes during validation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| /** | ||
| * @param {import('knex').Knex} knex | ||
| */ | ||
| exports.up = async function (knex) { |
Contributor
There was a problem hiding this comment.
P1: This migration runs ADD CONSTRAINT ... NOT VALID and VALIDATE CONSTRAINT in the same transaction, which can hold the stronger lock until commit and block writes during validation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/database/lib/migrations/20260604120250_sync_jobs_validate_id_big_not_null.cjs, line 4:
<comment>This migration runs `ADD CONSTRAINT ... NOT VALID` and `VALIDATE CONSTRAINT` in the same transaction, which can hold the stronger lock until commit and block writes during validation.</comment>
<file context>
@@ -0,0 +1,9 @@
+/**
+ * @param {import('knex').Knex} knex
+ */
+exports.up = async function (knex) {
+ await knex.raw(`ALTER TABLE nango._nango_sync_jobs ADD CONSTRAINT id_big_not_null CHECK (id_big IS NOT NULL) NOT VALID`);
+ await knex.raw(`ALTER TABLE nango._nango_sync_jobs VALIDATE CONSTRAINT id_big_not_null`);
</file context>
Contributor
Author
There was a problem hiding this comment.
check the description, we will run it separately in the cloud environment for that specific reason. But good catch!
Contributor
There was a problem hiding this comment.
Thanks, good catch!
TBonnin
approved these changes
Jun 4, 2026
…pau/nan-5491-phase3b-id-big-backfill
…-phase3c-id-big-unique-index
…5491-phase3d-validate-id-big-not-null
…order (NAN-5491 Phase 3a) Bumps the YYYYMMDD prefix from 20260604 to 20260608 so the migration sorts after the newer ones already on master (20260604200000_api_secrets_index_hashed, 20260605120000_drop_remote_functions_from_plans). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nd-id-big-column' into pau/nan-5491-phase3b-id-big-backfill
…order (NAN-5491 Phase 3b)
…fill' into pau/nan-5491-phase3c-id-big-unique-index
…order (NAN-5491 Phase 3c)
…ue-index' into pau/nan-5491-phase3d-validate-id-big-not-null
…order (NAN-5491 Phase 3d)
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
…-5491 Phase 3b) Match the value on master post #6496 — Phase 3a's branch still has the 3-day default in its history, so without this the squash-merge of 3b would silently revert master's default from 31 back to 3. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow the project's migration convention (rely on search_path); per review on #6377 — schema-qualifying the table name breaks custom-schema deployments that depend on search_path resolving to a different schema. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…fill' into pau/nan-5491-phase3c-id-big-unique-index
Follow the project's migration convention (rely on search_path). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ue-index' into pau/nan-5491-phase3d-validate-id-big-not-null
Follow the project's migration convention (rely on search_path). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Base automatically changed from
pau/nan-5491-phase3c-id-big-unique-index
to
master
June 22, 2026 09:51
nothingtosurprise
pushed a commit
to nothingtosurprise/nango
that referenced
this pull request
Jun 22, 2026
…e 3e) (NangoHQ#6380) ## Why Flip `_nango_sync_jobs.id` from `int4` to `bigint` in a single transaction, taking the table off the int4 exhaustion path — and keep the swap fully metadata-only by leaning on the validated CHECK from Phase 3d. ## How - **Stack:** depends on Phase 3d ([NangoHQ#6379](NangoHQ#6379)) — the `id_big_not_null` CHECK must be validated. - **Migrations: run manually** in cloud prod from psql, wrapped in `BEGIN; SET LOCAL lock_timeout = '1s'; SET LOCAL statement_timeout = '5s'; … COMMIT;`. Two safety nets: - `lock_timeout = '1s'` — bails if we can't acquire ACCESS EXCLUSIVE on the table within 1s (another long query is in the way). - `statement_timeout = '5s'` — every statement should be sub-millisecond catalog-only DDL. If something deviates (e.g., the PG 12+ CHECK-as-NOT-NULL-proof optimization unexpectedly doesn't kick in, causing a 168M-row scan), bail in 5s instead of waiting ~25s. Either timeout aborts the whole transaction atomically — no partial state. Migration row inserted into `nango._nango_auth_migrations` afterward to skip the auto-runner. - **Wait after merging:** **~24 hours** of clean operation before merging Phase 3f ([NangoHQ#6381](NangoHQ#6381)) — gives a rollback window in case the swap surfaces a subtle issue. The rollback procedure is documented below; once 3f lands and the sequence cap is lifted, that rollback becomes data-destructive. ### Commands (Schema-qualified for the operator runbook — the migration file itself uses unqualified table names and relies on `search_path`.) **1. Pre-flight checks** — confirm all 3a-3d preconditions hold: ```sql -- a) backfill complete (uses sync_jobs_id_big_uidx as Index Only Scan, sub-ms) SELECT count(*) FROM nango._nango_sync_jobs WHERE id_big IS NULL; -- expect: 0 -- b) unique index valid + ready SELECT indisvalid, indisready FROM pg_index WHERE indexrelid = 'nango.sync_jobs_id_big_uidx'::regclass; -- expect: t, t -- c) CHECK validated (this is what makes SET NOT NULL skip its scan inside the swap) SELECT convalidated FROM pg_constraint WHERE conrelid = 'nango._nango_sync_jobs'::regclass AND conname = 'id_big_not_null'; -- expect: t -- d) trigger still in place (it gets dropped inside the swap) SELECT tgname FROM pg_trigger WHERE tgrelid = 'nango._nango_sync_jobs'::regclass AND tgname = '_nango_sync_jobs_mirror_id_trigger'; -- expect: one row ``` **2. The swap** — single transaction with both timeouts: ```sql BEGIN; SET LOCAL lock_timeout = '1s'; SET LOCAL statement_timeout = '5s'; ALTER TABLE nango._nango_sync_jobs ALTER COLUMN id DROP DEFAULT; ALTER TABLE nango._nango_sync_jobs DROP CONSTRAINT _nango_sync_jobs_pkey; ALTER TABLE nango._nango_sync_jobs RENAME COLUMN id TO id_old; ALTER TABLE nango._nango_sync_jobs RENAME COLUMN id_big TO id; ALTER TABLE nango._nango_sync_jobs ALTER COLUMN id_old DROP NOT NULL; ALTER TABLE nango._nango_sync_jobs ALTER COLUMN id SET NOT NULL; ALTER TABLE nango._nango_sync_jobs DROP CONSTRAINT IF EXISTS id_big_not_null; ALTER TABLE nango._nango_sync_jobs ADD CONSTRAINT _nango_sync_jobs_pkey PRIMARY KEY USING INDEX sync_jobs_id_big_uidx; ALTER TABLE nango._nango_sync_jobs ALTER COLUMN id SET DEFAULT nextval('nango._nango_sync_jobs_id_seq'); ALTER SEQUENCE nango._nango_sync_jobs_id_seq OWNED BY nango._nango_sync_jobs.id; DROP TRIGGER IF EXISTS _nango_sync_jobs_mirror_id_trigger ON nango._nango_sync_jobs; DROP FUNCTION IF EXISTS nango._nango_sync_jobs_mirror_id(); COMMIT; ``` **3. Verify** — schema reflects the swap: ```sql -- a) New id is bigint NOT NULL with nextval default; id_old is int nullable SELECT column_name, data_type, is_nullable, column_default FROM information_schema.columns WHERE table_schema = 'nango' AND table_name = '_nango_sync_jobs' AND column_name IN ('id', 'id_old') ORDER BY column_name; -- expect: -- id | bigint | NO | nextval('nango._nango_sync_jobs_id_seq'::regclass) -- id_old | integer | YES | (null) -- b) PK now uses sync_jobs_id_big_uidx SELECT conname, pg_get_constraintdef(oid) FROM pg_constraint WHERE conrelid = 'nango._nango_sync_jobs'::regclass AND contype = 'p'; -- expect: _nango_sync_jobs_pkey | PRIMARY KEY (id) -- (PG hides the USING INDEX detail in the constraint def; the underlying index is sync_jobs_id_big_uidx) -- c) id_big_not_null CHECK is gone SELECT conname FROM pg_constraint WHERE conrelid = 'nango._nango_sync_jobs'::regclass AND conname = 'id_big_not_null'; -- expect: 0 rows -- d) Trigger + function are gone SELECT tgname FROM pg_trigger WHERE tgrelid = 'nango._nango_sync_jobs'::regclass AND tgname = '_nango_sync_jobs_mirror_id_trigger'; -- expect: 0 rows SELECT proname FROM pg_proc WHERE proname = '_nango_sync_jobs_mirror_id'; -- expect: 0 rows -- e) Sequence is owned by the new id column SELECT s.relname AS sequence, d.refobjsubid AS owned_by_attnum, a.attname AS owned_by_column FROM pg_class s JOIN pg_depend d ON d.objid = s.oid AND d.classid = 'pg_class'::regclass AND d.deptype = 'a' JOIN pg_attribute a ON a.attrelid = d.refobjid AND a.attnum = d.refobjsubid WHERE s.relname = '_nango_sync_jobs_id_seq'; -- expect: _nango_sync_jobs_id_seq | <attnum> | id ``` **4. Sanity check round-trip** against the live app — insert a sync_job through normal application path and confirm it gets a fresh bigint id from the sequence. **5. Mark migration applied** — skip the auto-runner on next deploy: ```sql INSERT INTO nango._nango_auth_migrations (name, batch, migration_time) VALUES ( '20260608120300_sync_jobs_id_atomic_swap.cjs', (SELECT COALESCE(MAX(batch), 0) + 1 FROM nango._nango_auth_migrations), NOW() ); ``` ### Mid-flight cancel (before COMMIT lands) The swap is a single transaction, so any error (lock_timeout, statement_timeout, constraint violation, manual cancel) **rolls back the whole thing atomically** — no manual cleanup needed. If you need to abort mid-flight from a separate psql session: ```sql -- Find the swap session: SELECT pid, now() - xact_start AS txn_duration, query FROM pg_stat_activity WHERE query ILIKE '%PRIMARY KEY USING INDEX sync_jobs_id_big_uidx%' AND pid <> pg_backend_pid(); -- Cancel it: SELECT pg_cancel_backend(<pid>); ``` After cancel/timeout, the table goes back to the pre-swap state. You can re-run the swap once whatever caused the issue is resolved. ### Last-resort post-commit rollback (only during the 24h bake window before Phase 3f) Once the swap has committed, true rollback requires undoing the column rename + restoring the PK on the int4 column. Two preconditions must hold: 1. **The sequence's `last_value` is still ≤ 2^31-1.** New bigint ids that have already been emitted must fit back into int4. After Phase 3f lifts the cap and a value > 2^31 is emitted, this rollback becomes data-destructive. 2. **Post-swap rows have NULL `id_old`** (the trigger that mirrored `id → id_big` was dropped). Step A below backfills them. ```sql -- ============================================================ -- Step A — backfill id_old for post-swap rows. Chunked, repeat -- until 0 rows match. Errors out if any id > 2^31-1. -- ============================================================ WITH batch AS ( SELECT ctid FROM nango._nango_sync_jobs WHERE id_old IS NULL LIMIT 15000 ) UPDATE nango._nango_sync_jobs s SET id_old = id::int FROM batch WHERE s.ctid = batch.ctid; -- Confirm: SELECT count(*) FROM nango._nango_sync_jobs WHERE id_old IS NULL; -- expect: 0 ``` ```sql -- ============================================================ -- Step B — build a unique index on id_old CONCURRENTLY (~5 min, -- non-blocking). Required because the original _nango_sync_jobs_pkey -- index was dropped during the swap, and the reverse swap needs an -- index to attach as the new PK. -- ============================================================ CREATE UNIQUE INDEX CONCURRENTLY sync_jobs_id_old_uidx ON nango._nango_sync_jobs (id_old); -- Verify: SELECT indisvalid, indisready FROM pg_index WHERE indexrelid = 'nango.sync_jobs_id_old_uidx'::regclass; -- expect: t, t ``` ```sql -- ============================================================ -- Step C — reverse swap (single txn, same timeout safety nets). -- ============================================================ BEGIN; SET LOCAL lock_timeout = '1s'; SET LOCAL statement_timeout = '5s'; ALTER TABLE nango._nango_sync_jobs ALTER COLUMN id DROP DEFAULT; ALTER TABLE nango._nango_sync_jobs DROP CONSTRAINT _nango_sync_jobs_pkey; ALTER TABLE nango._nango_sync_jobs RENAME COLUMN id TO id_big; ALTER TABLE nango._nango_sync_jobs RENAME COLUMN id_old TO id; ALTER TABLE nango._nango_sync_jobs ALTER COLUMN id_big DROP NOT NULL; ALTER TABLE nango._nango_sync_jobs ALTER COLUMN id SET NOT NULL; ALTER TABLE nango._nango_sync_jobs ADD CONSTRAINT _nango_sync_jobs_pkey PRIMARY KEY USING INDEX sync_jobs_id_old_uidx; ALTER TABLE nango._nango_sync_jobs ALTER COLUMN id SET DEFAULT nextval('nango._nango_sync_jobs_id_seq'); ALTER SEQUENCE nango._nango_sync_jobs_id_seq OWNED BY nango._nango_sync_jobs.id; COMMIT; ``` Notes: - The trigger/function are **deliberately not recreated** — at this point `id` is back to int4 and dual-writing isn't needed. `id_big` stays as a nullable orphan column you can `DROP COLUMN` later if desired. - If rolling back the data also requires reverting the merged PR, also remove the migration row so a re-run is clean: `DELETE FROM nango._nango_auth_migrations WHERE name = '20260608120300_sync_jobs_id_atomic_swap.cjs';` ## What - Atomic PK swap of `_nango_sync_jobs.id` from `int4` to `bigint`. All metadata-only DDL inside one transaction: drop default on old `id`, drop the PK, rename `id → id_old`, rename `id_big → id`, drop NOT NULL on `id_old`, SET NOT NULL on the new `id` (uses the validated CHECK as proof — no scan), drop the now-redundant `id_big_not_null` CHECK, attach the PK via `USING INDEX sync_jobs_id_big_uidx`, restore the `nextval` default, reassign sequence ownership to the new `id`, drop the mirror trigger + function. The sequence ceiling stays at `2^31-1` for now (Phase 3f ([NangoHQ#6381](NangoHQ#6381)) lifts it) — that's safe because the bigint column accepts those values without complaint and behavior is unchanged. - Schema-qualifier dropped in the migration file (`nango._nango_sync_jobs` → `_nango_sync_jobs`) to match the project convention of relying on `search_path` for table resolution. Linear: NAN-5491. ## Test plan - [ ] Cloud prod: pre-flight checks (NULL=0, index valid, CHECK validated, trigger present) - [ ] Cloud prod: run the swap block manually with `lock_timeout = '1s'` + `statement_timeout = '5s'` - [ ] Schema verifies (new bigint `id`, `id_old` int nullable, PK on new id, CHECK gone, trigger gone) - [ ] App round-trip insert succeeds - [ ] `INSERT INTO nango._nango_auth_migrations` to mark applied - [ ] Self-hosted auto-runs the migration on next deploy (no manual step needed there) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 free
to 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.
Why
Prove
id_bighas no NULL rows so Phase 3e's swap stays metadata-only underACCESS EXCLUSIVE— the long scan happens here, in a non-blocking lock mode.How
VALIDATEstep runs under its nativeSHARE UPDATE EXCLUSIVE(non-blocking) instead of inheritingACCESS EXCLUSIVEfrom the catalog write. The migration row is inserted intonango._nango_auth_migrationsafterward so the auto-runner skips it. Self-hosted runs the migration automatically (instant + small-table scan, no concern).Commands
(Schema-qualified for the operator runbook — the migration file itself uses unqualified table names and relies on
search_path.)1. Pre-flight check — backfill is still complete and unique index is in place:
2. Add the CHECK constraint (NOT VALID) — catalog entry only, wrapped with
lock_timeout = '1s'so the brief ACCESS EXCLUSIVE can't queue up behind a busy moment:3. Validate the constraint — runs OUTSIDE a transaction so it gets
SHARE UPDATE EXCLUSIVE(concurrent reads/writes keep flowing). Aborts the whole step if any row hasid_big IS NULL:4. Verify —
convalidatedmust bet:5. Mark migration applied — skip the auto-runner on next deploy:
Emergency stop (if VALIDATE needs to be cancelled mid-flight)
The
VALIDATEscan is interruptible since it runs underSHARE UPDATE EXCLUSIVE(no exclusive lock to release).1. Find the PID (from a separate psql session):
2. Cancel:
3. Clean up the unvalidated CHECK so a retry starts clean:
After the DROP, you're back to a clean pre-3d state and can retry from step 2.
What
CHECK (id_big IS NOT NULL)constraint namedid_big_not_nullto_nango_sync_jobs. From that point on, every INSERT/UPDATE is also constraint-checked, so the table can't drift back into containing NULLs while we wait to run 3e.ALTER COLUMN id SET NOT NULLbecomes metadata-only, andADD CONSTRAINT … PRIMARY KEY USING INDEXno longer has to scan underACCESS EXCLUSIVE. The redundant CHECK is dropped inside the swap right before the PK supersedes it.nango._nango_sync_jobs→_nango_sync_jobs) to match the project convention of relying onsearch_pathfor table resolution.Linear: NAN-5491.
Test plan
ADD CONSTRAINT … NOT VALIDwrapped withlock_timeout = '1s'VALIDATE CONSTRAINToutside a txnconvalidated = tonid_big_not_nullINSERT INTO nango._nango_auth_migrationsto mark applied