Most code review checklists are written for application code, and they miss what actually breaks in SQL. A migration that locks a live table during peak traffic. A new foreign key with no index behind it. A WHERE clause built by concatenating a string instead of binding a parameter. None of that shows up in a typical "readable variable names, no duplicated logic" review — SQL has its own failure modes, and the blast radius when one hits production is usually bigger than a bad application deploy.
Some of this is already solved by tooling. A formatter or linter catches inconsistent keyword casing, comma placement, and misspelled keywords before a human opens the diff — that's deterministic and doesn't need a second opinion. What's left for the reviewer is judgment: does this index actually need to exist, is this migration safe to run while the old application code is still deployed, can this dynamic query be exploited, and if the migration goes wrong, how do you get back. That's the review this checklist covers.
Below is the reasoning and runnable examples behind the items that trip teams up most — missing indexes, non-backward-compatible migrations, transaction safety, N+1 patterns, and unparameterized SQL — followed by a numbered checklist you can paste into a pull request template today.
What should SQL review catch that a linter can't?
A formatter can tell you that a keyword is lowercase when your team's convention is uppercase, or that COUTN should be COUNT. It can't tell you that a migration will lock a 40-million-row table for four minutes, or that a new query will start doing a sequential scan once the table grows past its current size. Keep those two review types separate: run formatting and typo checks in CI before a human ever looks at the diff, and spend the human review budget entirely on the judgment calls below.
If your team is still debating tabs vs. spaces or SELECT vs. select in pull request comments, that's a sign the mechanical checks aren't automated yet — see our guide to SQL formatting best practices and the case for uppercase vs. lowercase keywords. Fix that first; it's a bigger time save than anything else on this list, and it stops typo-class bugs like the ones covered in our rundown of common SQL keyword typos from reaching review at all.
Why does a new foreign key need an index?
This is dialect-specific, and it's an easy thing for a reviewer to miss. In PostgreSQL, declaring a foreign key constraint does not automatically create an index on the referencing column — the PostgreSQL documentation on foreign keys says so explicitly, and recommends indexing it anyway, because a DELETE on the referenced table or an UPDATE of a referenced column forces a scan of the referencing table to check for matches. MySQL's InnoDB engine goes the other way and creates that index for you automatically — one more entry in the list of MySQL vs PostgreSQL syntax differences that catches teams moving between the two.
The practical review rule: any ALTER TABLE ... ADD COLUMN ... REFERENCES (or a new table with a FOREIGN KEY clause) in a PostgreSQL migration needs a matching CREATE INDEX in the same diff. On a table with meaningful row counts, that index should be built with CREATE INDEX CONCURRENTLY so it doesn't hold a lock across the whole table while it builds.
How do you review a migration for backward compatibility?
A migration is backward compatible when it can run while the previous version of the application is still live — which is the normal state of the world during a rolling deploy. Renaming a column, dropping a column the old code still reads, or narrowing a type all break the currently running app the moment the migration completes, not when the new code deploys. The reviewer's job is to check whether a change is purely additive (safe) or a rename, drop, or type change (needs two steps).
The standard fix is the expand-contract pattern: add the new structure alongside the old one, ship application code that writes to both and reads from the new one, verify it in production, and only then drop the old column in a follow-up migration. It's also worth checking NOT NULL additions specifically — on PostgreSQL 11 and later, adding a column with a constant default no longer rewrites the table (the ALTER TABLE documentation confirms the default is stored in the table's metadata instead), so that part is cheap. What isn't cheap is adding NOT NULL with no default against a table that already has rows without one — that fails outright unless you backfill first.
What transaction-safety issues should a reviewer catch?
Any migration or data fix that touches more than one row or more than one statement needs to succeed or fail as a unit — otherwise a crash between statement one and statement two leaves data in a state no application code was written to handle. The review question is simple: if this fails halfway through, is the data still consistent? If not, it needs an explicit transaction.
Watch for the opposite failure too: a transaction held open across a slow network call, an external API request, or application logic will hold its locks for as long as it's open, which can stall every other write to that table. Keep the transaction scoped to just the statements that need atomicity, and keep long-running DDL, like an index build, out of a transaction that also does DML on the same tables.
How do you spot N+1 queries and other performance red flags?
The classic N+1 pattern is a query run once per row of an outer result set instead of once for the whole batch — fetch 50 orders, then run 50 separate queries for each order's customer. It's invisible in a unit test with three rows and expensive in production with three hundred thousand. In review, look for any query inside a loop in the application code, and ask whether it can be replaced with a single query using IN, a JOIN, or a batched load.
The other common red flag is a new query against a large table with no attached evidence it was tested at scale. Ask for an EXPLAIN ANALYZE excerpt in the PR description for any query touching a table over some size threshold your team agrees on — a sequential scan on a 10-row table is fine; the same plan on a 10-million-row table is an incident waiting to happen. SELECT * is a smaller version of the same problem: it pulls columns nobody asked for across the network and defeats covering indexes.
How do you catch SQL injection risk in dynamically built queries?
Most injection risk in a modern codebase isn't in hand-rolled SELECT statements — it's in the one place a query gets built dynamically: a search filter with an optional set of columns, a dynamic ORDER BY, an admin tool that builds a WHERE clause from a form. Any time a value is concatenated or interpolated directly into a SQL string instead of bound as a parameter, review it as if it were untrusted input, because eventually it will be.
The OWASP SQL Injection Prevention Cheat Sheet is the reference here, and the review rule it boils down to is: every value should be bound through a parameterized query or prepared statement, never concatenated into the SQL text. The one case parameters can't cover is a dynamic identifier — a table or column name chosen at runtime — since you can't bind a parameter where an identifier goes. Those need an allowlist checked against known-safe names, not sanitization.
The SQL code review checklist: 15 things to check before you approve
Paste this into your PR template and work through it on any change that touches schema or hand-builds a query:
Naming follows your team's convention for tables, columns, and constraints — and matches existing schema, not just itself.
Formatting and keyword casing are handled by a formatter or linter, not called out in review comments.
Every new foreign key column has a matching index in the same diff.
Every new index is justified by an actual query pattern, not added "just in case" — unused indexes slow down every write.
The migration is additive only, or uses expand-contract if it renames, drops, or narrows a type.
New NOT NULL columns either have a safe default or a documented backfill step run before the constraint is added.
Multi-statement writes that must succeed or fail together are wrapped in an explicit transaction.
No transaction is held open across a network call, external API request, or application logic.
Long-running DDL, like index builds, uses CONCURRENTLY or an online-DDL equivalent where the dialect supports it.
Queries against large tables include an EXPLAIN (or EXPLAIN ANALYZE) excerpt in the PR description.
No query loops per row where a single batched query would do — the N+1 check.
No SELECT * in application code paths that only need specific columns.
Every dynamically built query binds values as parameters; dynamic identifiers are checked against an allowlist.
New roles or connection strings follow least privilege — no new code path runs as a superuser.
The PR or its migration has a rollback path: a down-migration, a documented manual undo, or an explicit note that the change is irreversible and why that's acceptable.
Wrapping up
SQL code review is mostly judgment that automation can't make for you yet: whether an index is worth its write cost, whether a migration is safe mid-deploy, whether a dynamic query can be exploited. Run the checklist above on every PR that touches your schema or writes raw SQL, and it catches the failures that "does it run" testing misses.
Keep the mechanical half — casing, comma style, and keyword typos — out of the conversation entirely by running SQL through a formatter and validator first, like sqlfmt, so reviewers spend their time on the fifteen items above instead of nitpicking style. If your whole team wants that check running live as they type rather than on demand, that's what the Pro plan is for.