Uh oh!
There was an error while loading. Please reload this page.
perf(db): drop 0287's two zero-row scans from the ACCESS EXCLUSIVE hold - #6609
Conversation
drizzle-orm 0.45.2 runs every pending migration inside ONE transaction (node_modules/drizzle-orm/pg-core/dialect.js:60-71), and migrate.ts:212 sets `statement_timeout = 0`. So the ACCESS EXCLUSIVE that 0287:2's ADD COLUMN takes on `workflow_blocks` is held, unbounded, through 0288 and 0289 to COMMIT — `lock_timeout` at migrate.ts:213 bounds acquisition only, exactly as that file's own TSDoc at :76-78 says. Every editor load, workflow save, executor block read, and realtime canvas op queues behind it platform-wide, and migrations run before image promotion (ci.yml:113-133), so the stall lands on 100% old-version traffic. Two of the three statements inside that hold did nothing. `data.errorEnabled` never existed in a released version — `git log origin/main -S errorEnabled` returns zero commits across main's entire history — so both statements filtered on it match zero rows, and the file's own comment said as much. There is no index on the `data` expression, so each was a full sequential scan of the whole table. Measured on PostgreSQL 17.9 against a 328 MB / 200k-row fixture built to the same bytes-per-row shape as the reported production table: 0287:2 ADD COLUMN 0.5 ms metadata-only, takes AccessExclusiveLock 0287:11 edge backfill 21 ms Nested Loop -> Index Scan on the PK 0287:20 (deleted) 47 ms Seq Scan, 200,000 rows removed, 0 matched 0287:22 (deleted) 47 ms Seq Scan, 200,000 rows removed, 0 matched A concurrent primary-key SELECT started 50 ms into the transaction was blocked 53-219 ms before and 22-25 ms after — the latter indistinguishable from the 21-33 ms control with no migration running at all. The two deleted statements accounted for 80,520 buffer accesses (~629 MB of in-lock I/O on that fixture) and 81% of the transaction's work. Editing an already-merged migration in place is safe here specifically because drizzle writes `hash` but never reads it back: the skip test at dialect.js:56-63 compares `created_at` against `folderMillis` only. The edit is therefore a no-op on every database that already applied 0287 (staging, dev, branch DBs) and takes effect only where it has not run. `meta/_journal.json` and the snapshot prevId chain are untouched. The only casualty is a stale `data.errorEnabled` key on branch databases a developer created it on. Nothing reads it: save.ts:50 and load.ts:89 both read the `error_enabled` COLUMN, and load.ts passes `data` through untouched. Alternatives rejected, each checked against the code rather than assumed: - An embedded `COMMIT;` to end the transaction early. 0289's four CREATE TYPE, its CREATE TABLE, and its three CREATE INDEX all lack IF NOT EXISTS, so a mid-batch failure in autocommit leaves them applied-but-unjournaled and the replay dies on 42710 — which migrate.ts:219 only retries for 55P03. That turns a transient stall into a wedged deploy. - Moving the statements to a new file after 0289. All pending files share one transaction, so it buys exactly zero lock reduction. - Rewriting the surviving backfill as `WHERE id IN (SELECT source_block_id FROM workflow_edges WHERE ...)`. Measured both: planner-equivalent. `source_handle` is unindexed, so both forms seq-scan `workflow_edges` (7.5 ms, identical) and then index-scan `workflow_blocks` on the primary key — it never scans that table. The IN form adds a HashAggregate and 1,170 more buffers, so it is marginally worse. Left as shipped. - Promoting the `data-backfill` lint from warn to annotate. `readAnnotation` only requires a non-empty reason and 0287 already supplies one per statement, so the rule would fire zero findings. 0288's nullable `retry` column is correct as-is and unchanged. Both delete-and-reinsert save paths on the deployed version (save.ts:30-63 and the realtime REPLACE_STATE handler) reset `error_enabled` to false and `retry` to NULL for a workflow saved by an old replica during the rollout; the ordinary realtime block upsert does not, because its `set` clause omits both columns. That residue is tolerable: everything the surviving backfill writes is re-derived from the edge set at workflow-block.tsx:754 and lib/workflows/persistence/utils.ts:196-201, and `retry` ships in this same release so it has no installed base. `bun run check:migrations origin/main` drops from three data-backfill warnings to one. The real migrator applied all 289 journal entries to a fresh PostgreSQL 17.9 database with the edited file, producing `error_enabled boolean not null default false` and `retry jsonb`.
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview Keeps the real edge-driven backfill that sets Reviewed by Cursor Bugbot for commit 7e6f936. Configure here. |
Greptile SummaryRemoves two zero-row JSONB scans from migration 0287, shortening the
Confidence Score: 5/5The PR appears safe to merge because the removed statements target a JSONB key unused by current persistence paths, and the required edge-driven backfill remains unchanged. Current reads and writes use the dedicated
|
| Filename | Overview |
|---|---|
| packages/db/migrations/0287_workflow_blocks_error_enabled.sql | Safely removes two obsolete full-table scans while retaining the schema change and required edge-based reconciliation. |
Reviews (1): Last reviewed commit: "perf(db): drop 0287's two zero-row scans..." | Re-trigger Greptile
Summary
Migration 0287 adds
workflow_blocks.error_enabledand then ran three backfill statements. Two of them scanned the whole table to match a jsonb key,data ->> 'errorEnabled', that only ever existed on an unmerged branch. This deletes those two and keeps the one real edge-driven backfill.The two deleted statements could never have matched anything
The key was never written by any released version, confirmed two ways:
git log origin/main -S"errorEnabled"is empty — the key never shipped. It existed only on an unmerged branch, and the squash that landed the feature (feat(workflows): new workflow block card, progress indicator, colors, dsl for natural language preview, retry configs #6458) writes only the column.errorEnabledreference in the tree is the Drizzle field mapping to theerror_enabledcolumn (packages/db/schema.ts:297), never a key inside thedatajsonb.BlockDatahas no such field, andBlockState.errorEnabledis a sibling ofdata, never nested inside it. All five persistence writers —workflow-persistence/src/{save,load}.tsand the realtimeUPDATE_ERROR_ENABLED/ upsert / full-state-replace paths — set the column only.So both statements filtered on
data ->> 'errorEnabled', which is unindexable without an expression index and therefore forces a sequential scan that detoasts the jsonb payload of a large, hot table — to match nothing.Why that cost mattered more than it looks
Two things compound it:
statement_timeout = 0. TheACCESS EXCLUSIVElock thatALTER TABLE ... ADD COLUMNtakes onworkflow_blocksis held from that statement through 0288 and 0289 all the way toCOMMIT. Both deleted scans ran inside that lock window, with no statement timeout to cut them off — every reader and writer ofworkflow_blocksblocks behind them.The retained backfill is driven from
workflow_edgesand resolves blocks through the primary key index, so it never reads or detoastsdata. It is by far the cheaper plan and touches only the small subset of blocks that already have an error edge. Verified against a production-shaped database: the deleted statements matched nothing, and the retained one planned as an index-driven join.This lands before the migration has run anywhere it matters, so there is no cleanup debt and nothing to reconcile.
Why it is safe to edit an already-merged migration
0287 is already on
staging(it landed in #6458). Editing the file is safe because drizzle's postgres-js migrator gates purely on timestamp, not on the file hash (drizzle-orm/pg-core/dialect.cjs:if (!lastDbMigration || Number(lastDbMigration.created_at) < migration.folderMillis)). The hash is stored but never compared, so an environment that already applied 0287 skips the edited file silently rather than failing on a hash mismatch, and an environment that has not yet applied it runs the shortened version.meta/_journal.jsonis untouched, so ordering against 0288 and 0289 is preserved either way.The retained backfill is load-bearing — it is not part of the cleanup
Every released version draws the error port with no toggle in front of it, so a block someone already wired an error edge out of has the output on. Defaulting those rows to
falsewould hide a port live workflows route failures through.workflow-block.tsx:754(Boolean(currentBlock?.errorEnabled || hasErrorConnection)) is the render-time twin of the same rule. This statement keeps its-- migration-safe:acknowledgment and its explanatory comment.Rejected alternatives
COMMIT;to break the batch. Shortens the lock hold but risks a wedged deploy. 0289 creates four enum types, a table, and three indexes, none withIF NOT EXISTS(Postgres does not support it onCREATE TYPEat all). With a mid-batchCOMMIT, a later failure leaves those objects applied but the journal unwritten, and the replay dies on42710 duplicate_object— turning a transient stall into a deploy that cannot move without manual intervention.ACCESS EXCLUSIVEhold.CREATE INDEX CONCURRENTLYto support them. Not applicable — the statements are gone. Batching zero matched rows still requires the same full scans to discover there is nothing to do.WHERE EXISTS (...). Avoids duplicate join rows, but a semi-join drives fromworkflow_blocksinstead of primary-key lookups — a worse plan. Left as-is.Scope
No
COMMIT;introduced, not renumbered,meta/_journal.jsonuntouched, noschema.tschange implied. The retained statement is now correctly the final one, with no dangling--> statement-breakpoint.One residual: a developer machine that ran the unmerged branch may still have
data->>'errorEnabled'set locally, which will no longer be migrated onto the column or stripped. This is self-healing — the save path writesdatawholesale from the client payload (data: sql`excluded.data`inapps/realtime/src/database/operations.ts) anderrorEnabledis a top-levelBlockStatefield, so the next save of that workflow drops the stale key and writes the column. No row outside a developer's own machine can be in that state.Type of Change
Testing
bun run scripts/check-migrations-safety.tspasses — no blocking issues, one acknowledged data-backfill warning on the retained statement. Behavior was verified against a production-shaped database with read-only queries andEXPLAIN.Checklist