Code Review

How to Review a Database Migration

A migration file that looks clean in review can still take down a production database at 2 a.m. The reviewer's job is to find the failure that has not happened yet, and the only way to do that reliably is to read the migration as a sequence of operations that will run against real rows, under real locks, on a server that is already busy.

Most code review habits collapse the moment a migration appears in the diff. A reviewer scans the SQL, checks the syntax, confirms the table name, and approves. That habit works for application code because a bad function call throws an error in a test suite. A migration does not throw. It waits until the deploy window, then it runs against a table with forty million rows and a replica that is two seconds behind, and the error arrives as a timeout on a page that has nothing to do with the change.

Read the Down Migration Before the Up

The rollback is where migrations reveal their honesty. Open the down section first. If it is empty, missing, or a comment that says "irreversible," the review has already found its first problem. A migration that cannot be reversed is a one-way door, and one-way doors deserve a conversation before they are merged, not after.

A down migration that drops a column and its data is technically reversible in schema and permanently destructive in content. That distinction matters. The reviewer should ask what the down migration restores and what it loses, and whether the team has agreed to that loss in writing. A comment in the pull request saying "we can restore from backup" is a plan, and plans get tested during incidents, which is the worst time to discover the backup restore takes four hours.

There is a quieter failure in the down section: a rollback that references a column the up migration renamed. The down runs, the column is gone, the rollback errors midway, and the schema is now in a state that neither the old code nor the new code expects. Reviewers catch this by reading the two directions side by side and tracing each identifier through both.

Locks Are the Real Cost of a Schema Change

Adding a column to a large table is fast on an empty development database and slow on a production table with active writes. The reason is the lock. A schema change takes an exclusive lock on the table, and that lock queues behind every open transaction touching the table, and every new query queues behind the lock. One long-running report can turn a two-second migration into a ten-minute outage.

The reviewer's move here is to ask what lock the operation takes and how long it holds. Some databases allow an index to be created without blocking writes, using a concurrent build; others do not, and the same migration that is safe on one engine will freeze the other. The migration file alone does not always answer this, which is why the review should include a note about the operations that need a lock and the expected table size. A migration with no comment about lock behavior on a table above a few million rows is a migration reviewed on faith.

Column defaults deserve a specific look. On older database versions, adding a column with a default value rewrites every row, which means a full table scan and a long lock. Newer versions handle this without the rewrite, but only for certain default types and only above a specific version. The reviewer who does not know the production version cannot judge whether the default is cheap or catastrophic.

The Deploy Order Between Code and Schema

A migration and the application code that uses it rarely ship in the same instant. There is a window, sometimes seconds, sometimes an hour during a rolling deploy, when old code runs against new schema or new code runs against old schema. The migration review has to name which side of that window the change lands on.

Three patterns cover most cases. Additive changes, like a new nullable column, are safe in either order: old code ignores the column, new code fills it. Destructive changes, like dropping a column, are only safe after every running instance of the old code has stopped reading it, which means a two-step deploy. Renames are the dangerous middle: old code reads the old name, new code reads the new name, and during the window one of them fails.

A rename should be split into a copy, a backfill, a code switch, and a cleanup, spread across releases. The reviewer who sees a rename and a code change in the same pull request should flag the deploy window explicitly and ask whether a rolling deploy is in use. A migration is safe in isolation and dangerous in sequence, and the sequence is what review is for.

Backfills Need Batching and a Throttle

A backfill that runs as a single update statement over an entire table is the most common way to accidentally take a database offline. It holds locks for the duration, it bloats the write-ahead log, and it competes with production traffic for I/O. The reviewer should look for a loop that processes rows in chunks, with a small sleep between batches, and a way to resume if the process dies halfway.

Batching also needs an index on the column being used to paginate. A backfill that walks a table by an unindexed timestamp will scan from the start on every batch and get slower as it progresses. The reviewer can check this by finding the where clause of the batch query and confirming an index exists that supports it.

Resumability is the part teams skip. A backfill script that starts at the beginning every time it runs will, after a crash at ninety percent, redo ninety percent of the work and double the load. A simple marker, a last processed id or a timestamp, turns a restart into a continuation. The reviewer should find that marker in the file or ask why it is missing.

Nullability Changes Are Data Changes in Disguise

Adding a not null constraint to a column that already allows nulls fails if any existing row holds a null. The migration file cannot know that, so the reviewer has to ask whether the data has been checked. A migration that adds the constraint without a prior update to fill nulls is a deploy that fails on the first offending row, and production data is exactly where the offending rows live.

The order matters here: fill the nulls, verify the count of remaining nulls is zero, then add the constraint. A migration that does all three in one transaction locks the table for the duration of the fill. A careful migration separates them, often into two deploys, so the locking operation only runs after the data is already clean.

Foreign keys carry a similar trap. Adding a foreign key validates every existing row against the referenced table, which is a scan on both sides. On large tables this can run for minutes. Some databases allow the constraint to be added without validation and validated separately later, which splits the cost. The reviewer should know which form the migration uses and why.

Indexes Built in the Same Transaction

Creating an index inside the same transaction as the rest of the migration means the index build holds a lock for its entire duration and cannot be interrupted. On a large table, a single index build can take longer than the deploy window allows, and the transaction rolls back, leaving the schema unchanged and the deploy failed.

The concurrent index build, where the database supports it, allows writes during the build but takes longer overall and can fail on its own if a conflicting transaction holds a lock. The reviewer should check that concurrent builds are not wrapped in a transaction block, since some databases reject that combination outright.

A migration that adds three indexes to one large table in one file deserves a second look at timing. Three concurrent builds, run back to back, can each take minutes, and the deploy pipeline may time out before the third finishes. Splitting them across migrations lets each one succeed or fail on its own.

The File Should Say What It Expects

A migration with a header comment stating the target table, the estimated row count, the expected lock duration, and the rollback plan is a migration that was thought about. The absence of that comment is not proof of a bad migration, but on a table of any size it is a reason to ask questions before approving.

The reviewer can also check that the migration is idempotent where the framework allows it. A migration that fails halfway and cannot be re-run cleanly forces a manual cleanup during an incident, and manual cleanup during an incident is where mistakes compound. Guards like an existence check before creating a table or a column turn a re-run into a safe operation.

None of this requires the reviewer to be a database administrator. It requires reading the file as a set of operations that will execute against real rows on a real server, and asking the questions that the file itself does not answer. The migration that passes review is not the one that looks tidy. It is the one whose author can explain, line by line, what happens when it runs against a table with forty million rows and a replica that is two seconds behind. When the reviewer cannot get that explanation, the approval should wait, because the deploy will not.

Leave a Reply

Your email address will not be published. Required fields are marked *

Back to top button