A pull request full of application code gets three reviewers and a linter. The migration that ships alongside it, the one that will lock a hot table or quietly multiply a report by three, often gets a thumbs-up emoji. SQL deserves its own review lens, and this guide is that lens.
Reviewing SQL is not the same skill as reviewing Python or Go. A subtle SQL defect does not throw an exception in CI; it returns a plausible number that is wrong, or it runs in 40 milliseconds on your laptop against 500 rows and 40 seconds in production against 50 million. This guide gives developers and tech leads a repeatable way to review queries, stored procedures, and migrations, organized around seven concerns with concrete good-versus-bad examples for each.
What this guide covers
- Why SQL needs its own review lens
- Correctness: NULLs, GROUP BY, join fan-out
- Security: injection and least privilege
- Performance: sargability, SELECT *, N+1
- Transactions and error handling
- Readability and naming
- Set-based vs row-by-row
- Testing and idempotent migrations
- Reviewing stored procedures and migrations in depth
- Tooling: linters, EXPLAIN in CI, plan regression
- Team conventions and PR etiquette for SQL
- The review checklist
- Review with EXPLAIN, not opinions
- A reviewer's rubric
Why SQL deserves its own review lens
Most code review habits assume that wrong code fails loudly. SQL breaks that assumption in three ways, and a good reviewer keeps all three in mind.
- Wrong answers look right. A query with a bad join returns rows, a number, a report. Nothing crashes. The only symptom is that finance says the total is off, three weeks later.
- Cost is invisible at review time. The query in the diff has no row count attached. A missing index or a non-sargable predicate is free on the reviewer's test data and catastrophic at production scale. See our guide to performance tuning for how those costs compound.
- The blast radius is the whole dataset. A one-line application bug affects one request. A one-line migration bug can rewrite or delete every row in a table, and it holds locks while it does so.
Because of this, SQL review is less about style and more about asking "what happens when this meets real data and real concurrency?" The sections below turn that question into specific checks.
There is a cultural dimension too. On many teams, whoever wrote the query is the only person who fully understands the schema it touches, so review degrades into a rubber stamp. The antidote is not more heroics from one database expert; it is a shared checklist that any competent engineer can apply, plus tooling that makes the objective facts (the query plan, the lock behavior, the grant) visible in the pull request itself. When the review is grounded in evidence rather than seniority, the whole team gets better at SQL, and the bus factor stops being one. Treat every SQL diff as a small production change, because that is exactly what it is.
Correctness: NULLs, GROUP BY, and join fan-out
Correctness bugs are the most expensive because they ship silently. Three patterns account for most of them.
NULL handling
NULL is not a value; it is the absence of one. Any comparison with =, <>, or NOT IN against a nullable column is a landmine.
-- Rows where status is NULL are silently excluded from BOTH branches
SELECT id FROM orders WHERE status <> 'shipped';
-- If sub_ids contains a single NULL, this returns ZERO rows, always
SELECT id FROM users
WHERE id NOT IN (SELECT user_id FROM bans);-- Be explicit about how NULL status should be treated
SELECT id FROM orders
WHERE status IS DISTINCT FROM 'shipped'; -- Postgres
-- NOT EXISTS is NULL-safe where NOT IN is not
SELECT u.id FROM users u
WHERE NOT EXISTS (
SELECT 1 FROM bans b WHERE b.user_id = u.id);In review, flag every NOT IN against a subquery and ask whether the column can be NULL. If it can, request NOT EXISTS.
GROUP BY correctness
Some engines (older MySQL with a permissive sql_mode) let you select columns that are not in the GROUP BY and are not aggregated. The engine picks an arbitrary row. The result is non-deterministic.
-- customer_name is not grouped and not aggregated -> arbitrary value
SELECT customer_id, customer_name, SUM(total)
FROM orders
GROUP BY customer_id;SELECT customer_id, customer_name, SUM(total)
FROM orders
GROUP BY customer_id, customer_name;Join fan-out (multiplication)
The classic silent correctness bug: joining to a one-to-many table before aggregating, so every parent row is counted once per child.
-- Each order line multiplies the order; SUM(o.total) is inflated
SELECT o.id, SUM(o.total) AS revenue
FROM orders o
JOIN order_items i ON i.order_id = o.id
GROUP BY o.id;-- Aggregate the child first, then join at the right grain
SELECT o.id, o.total AS revenue, i.item_count
FROM orders o
JOIN (
SELECT order_id, COUNT(*) AS item_count
FROM order_items GROUP BY order_id
) i ON i.order_id = o.id;Reviewer heuristic: whenever you see an aggregate over a join, ask "what is the grain of each row before the GROUP BY?" If a join can multiply rows, the aggregate is suspect until proven otherwise. Our roundup of common SQL mistakes covers more of these.
Security: injection and least privilege
SQL injection remains one of the most common critical defects, and it is almost always visible in the diff. The rule is simple: data never becomes code. User input goes into parameters, never into the string that is compiled as SQL.
// String concatenation: one apostrophe and the query is theirs
sql = "SELECT * FROM users WHERE email = '" + email + "'";
-- What the attacker sends as email:
-- ' OR '1'='1' --// Parameterized: the driver sends SQL and values separately
sql = "SELECT id, email FROM users WHERE email = ?";
stmt.setString(1, email);Reject any patch that builds SQL by concatenation of request data, string interpolation, or format strings, in any language. This includes dynamic ORDER BY and table names, which cannot be parameterized and must instead be validated against an allow-list.
// Column comes from a query-string param -> injectable
sql = "SELECT * FROM products ORDER BY " + sortCol;// Map the untrusted key to a known-safe column name
allowed = { "price": "price", "name": "name" };
col = allowed[sortCol] || "name";
sql = "SELECT * FROM products ORDER BY " + col;Least privilege
Injection is worse when the application connects as a superuser. Review the grant, not just the query. An application account should hold only the rights it needs.
GRANT ALL PRIVILEGES ON *.* TO 'app'@'%';GRANT SELECT, INSERT, UPDATE, DELETE
ON shop.* TO 'app'@'10.0.%';For a deeper treatment of hardening data access, see our SQL security guide. In review, treat a new broad GRANT the same way you treat a disabled auth check.
Performance: sargability, SELECT *, and N+1
Performance defects hide because they are cheap on small data. Four patterns are worth catching on sight.
Sargability
A predicate is sargable (Search ARGument able) if the engine can use an index for it. Wrapping the indexed column in a function or doing arithmetic on it defeats the index.
-- Function on the column: index on created_at is unusable
WHERE DATE(created_at) = '2026-07-04';
-- Leading wildcard: index cannot seek
WHERE email LIKE '%@gmail.com';-- Range on the raw column keeps the index seekable
WHERE created_at >= '2026-07-04'
AND created_at < '2026-07-05';When you see a function applied to a column in a WHERE or JOIN, ask whether it can be rewritten as a range, or whether the schema needs a functional index. Our note on SQL indexes explains when each is appropriate.
SELECT * in production code
SELECT * is fine at the psql prompt and wrong in shipped code. It ships columns nobody uses (more I/O and network), breaks the instant a column is added or reordered, and prevents covering-index-only scans.
SELECT * FROM users WHERE id = ?;SELECT id, email, display_name FROM users WHERE id = ?;N+1 queries
The N+1 pattern usually lives in application code but shows up as SQL in the diff or the query log: one query to fetch a list, then one query per item. Ten items is fine; ten thousand is an outage.
-- 1 query for the list...
SELECT id FROM orders WHERE customer_id = ?;
-- ...then, in a loop, 1 query PER order:
SELECT * FROM order_items WHERE order_id = ?;-- One query, joined or batched by IN
SELECT i.* FROM order_items i
JOIN orders o ON o.id = i.order_id
WHERE o.customer_id = ?;Missing-index awareness is the fourth check: if a new query filters or joins on a column, ask whether an index exists to support it. You do not need to memorize the plan, but you should ask the author to attach the EXPLAIN output. More patterns live in our performance tuning reference.
Transactions and error handling
Stored procedures and multi-statement changes must be reviewed for atomicity. Two writes that must both succeed belong in one explicit transaction with a rollback path.
-- No transaction: a crash between these leaves money missing
UPDATE accounts SET balance = balance - 100 WHERE id = 1;
UPDATE accounts SET balance = balance + 100 WHERE id = 2;BEGIN;
UPDATE accounts SET balance = balance - 100 WHERE id = 1;
UPDATE accounts SET balance = balance + 100 WHERE id = 2;
COMMIT;In a stored procedure, that means a handler that rolls back and re-raises rather than swallowing the error.
-- MySQL: roll back on any SQL exception, then re-signal
DECLARE EXIT HANDLER FOR SQLEXCEPTION
BEGIN
ROLLBACK;
RESIGNAL;
END;
START TRANSACTION;
-- ... work ...
COMMIT;Watch out: under high concurrency, transactions will hit deadlocks no matter how careful the ordering. Any write path that can deadlock needs a retry loop with a small backoff, and locks acquired in a consistent order across procedures. A procedure that assumes it never deadlocks is a bug waiting for load. See stored procedures and functions for the full pattern.
Readability and naming
Unreadable SQL is a correctness risk because reviewers cannot see the bug. Consistent formatting, meaningful aliases, and comments on intent are not cosmetic; they are how the next person avoids breaking it.
select a.id,b.n,c.t from orders a,customers b,items c
where a.cid=b.id and c.oid=a.id and a.st=1;-- Active orders with their customer and line items
SELECT o.id AS order_id,
cust.name AS customer_name,
item.title AS item_title
FROM orders o
JOIN customers cust ON cust.id = o.customer_id
JOIN items item ON item.order_id = o.id
WHERE o.status = 1; -- 1 = activeAsk for: explicit JOIN ... ON syntax (never comma joins), keywords in a consistent case, table aliases that mean something rather than a/b/c, and a comment for any magic number or non-obvious filter. Naming conventions should be a written standard, not a per-author preference.
Set-based vs row-by-row thinking
SQL is a set language. A cursor that loops row by row (sometimes called RBAR, row by agonizing row) is usually an anti-pattern hiding a single statement. When you see a cursor in a review, the default question is "why is this not one UPDATE?"
-- Cursor to give active users a 10% credit: slow and verbose
DECLARE c CURSOR FOR SELECT id FROM users WHERE active = 1;
-- open, fetch loop, UPDATE one row at a time, close...-- One set-based statement the optimizer can plan
UPDATE users
SET credit = credit * 1.10
WHERE active = 1;Cursors have legitimate uses (calling a per-row procedure with side effects, or chunking a huge update to limit lock duration), but those should be justified in the description, not the default.
Testing and idempotent migrations
A migration is code that runs once, in production, often under lock. It should be reviewed harder than anything else. Two properties matter: it must be reversible or forward-safe, and re-running it must not blow up.
-- Fails on the second run; no guard, no down path
ALTER TABLE users ADD COLUMN last_login TIMESTAMP;
CREATE INDEX idx_login ON users(last_login);-- Idempotent guards make re-runs safe
ALTER TABLE users
ADD COLUMN IF NOT EXISTS last_login TIMESTAMP;
CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_login
ON users(last_login); -- CONCURRENTLY avoids a long write lockReview migrations for: a tested rollback (or a documented reason there is none), locking behavior on large tables (prefer CONCURRENTLY or batched backfills), data backfills separated from schema changes, and a dry run against a production-sized copy. A migration without a rollback plan is a deploy without a seatbelt.
Reviewing stored procedures and migrations in depth
Queries embedded in application code get exercised by integration tests and fail loudly when they break. Stored procedures and migrations do not have that safety net. A procedure is a long-lived object that many callers depend on, and a migration runs exactly once against live data, often behind a lock. Both deserve a slower, more skeptical read than an ordinary query, and both reward a small set of dedicated questions.
Stored procedures: contracts, not scripts
A stored procedure is an API. Its signature, its side effects, and the invariants it assumes are a contract that callers rely on, so a change to a procedure is a change to a published interface. When you review one, read it the way you would review a public function in a shared library.
- Parameter validation. Does the procedure guard against NULL or out-of-range inputs, or does it silently do the wrong thing? A procedure that trusts its arguments is a procedure that will corrupt data when a caller passes a stale id.
- Transaction boundaries. Does the procedure open its own transaction, or does it assume the caller already has one open? Mixing the two models causes writes to commit at surprising times. Make the ownership explicit and document it in a header comment.
- Error contract. On failure, does it roll back and re-raise, or swallow the error and return a misleading success? Silent failure inside a procedure is one of the hardest defects to trace back to its source.
- Idempotency of effects. If the same call is retried after a timeout, does it double-charge, double-insert, or does it detect the prior effect? Retries are a fact of distributed life; procedures with side effects need to survive them.
-- A procedure header that states its contract up front
-- credit_account(acct BIGINT, amount NUMERIC, ref TEXT)
-- Caller must NOT hold an open transaction.
-- Idempotent on ref: a repeated ref is a no-op.
-- Raises 'insufficient_funds' or 'unknown_account'.
INSERT INTO ledger(acct, amount, ref)
VALUES (acct, amount, ref)
ON CONFLICT (ref) DO NOTHING; -- retry-safe by designFor the mechanics of writing procedures that behave under load, our reference on stored procedures and functions goes deeper. In review, the single most valuable habit is to insist on that header comment: it forces the author to state the contract, and it gives the next reviewer something to check the body against.
Migrations: reversibility and lock awareness
Section eight covered idempotent guards; the deeper migration review is about two things the guards do not solve: whether you can undo the change, and what it does to concurrent traffic while it runs. A migration that is correct in isolation can still take a production database offline if it takes an exclusive lock on a hundred-million-row table during peak hours.
-- Rewrites every row and holds a full table lock the whole time
ALTER TABLE events ALTER COLUMN payload TYPE JSONB
USING payload::JSONB;
-- NOT NULL on a huge table also scans and locks
ALTER TABLE events ALTER COLUMN user_id SET NOT NULL;-- Add nullable, backfill in batches, then validate cheaply
ALTER TABLE events ADD COLUMN payload_jsonb JSONB;
-- backfill in a loop of small ranges, outside one big txn:
UPDATE events SET payload_jsonb = payload::JSONB
WHERE id BETWEEN :lo AND :hi
AND payload_jsonb IS NULL;
-- add a CHECK NOT VALID, then VALIDATE without a long lock
ALTER TABLE events
ADD CONSTRAINT user_id_nn CHECK (user_id IS NOT NULL) NOT VALID;
ALTER TABLE events VALIDATE CONSTRAINT user_id_nn;When reviewing a migration, ask five questions in order: Is there a down migration, and has it been run? Does any statement take a lock stronger than SHARE on a large table? Are data backfills separated from schema DDL so a slow backfill cannot hold a schema lock? Has it been dry-run against a production-sized copy with realistic timing? And is it forward-compatible with the currently deployed application code, so a rolling deploy does not break mid-flight?
Watch out: "reversible" and "reverted safely" are different claims. Dropping a column reverses the schema but destroys the data in it, so the down migration is not a true undo. Flag any irreversible step and make sure the team accepts the one-way door on purpose, with a backup taken first, rather than by accident.
Tooling: linters, EXPLAIN in CI, and plan regression
Humans are good at judgment and bad at consistency. The reverse is true of tools. The most effective SQL review process automates the mechanical checks so that human reviewers can spend their attention on correctness and design instead of arguing about formatting or re-discovering the same missing-index problem every sprint.
SQL linters and formatters
A linter enforces the conventions in this guide before a human ever opens the diff. Tools such as sqlfluff, sqlint, and database-native analyzers can flag comma joins, inconsistent keyword casing, SELECT *, tab-versus-space drift, and unqualified column references. Wire the formatter into a pre-commit hook and the linter into CI so that style is decided by configuration, not by whoever reviews that day. Once formatting is automatic, review comments stop being about whitespace and start being about behavior.
# Fail CI on lint violations; auto-fix locally on commit
sqlfluff lint --dialect postgres migrations/ queries/
sqlfluff fix --dialect postgres queries/Running EXPLAIN in CI
The most valuable automation is capturing the query plan for every changed query in continuous integration. Point CI at a database seeded with production-like statistics, run EXPLAIN on each query the change touches, and fail the build when a plan contains a red flag, such as a sequential scan on a table above a size threshold or an estimated cost above a budget. This turns "I think this will be slow" into an automated gate that never forgets to check.
-- CI harness runs this and parses the JSON output
EXPLAIN (FORMAT JSON, BUFFERS)
SELECT id, email FROM users WHERE email = 'x@example.com';
-- assertion: no node has "Node Type": "Seq Scan" on usersQuery plan regression checks
Plans drift. A query that used an index seek last quarter can flip to a full scan after the data grows, the statistics go stale, or someone drops an index in an unrelated cleanup. A plan-regression test stores a baseline plan (or a key metric like estimated rows or chosen access method) and fails when a new commit changes it for the worse. This is the SQL equivalent of a performance snapshot test, and it catches the slow, silent decay that no single review can see.
| Tool layer | Catches | When it runs |
|---|---|---|
| Formatter (pre-commit) | Casing, indentation, comma joins | On commit |
| Linter (CI) | SELECT *, unqualified columns, anti-patterns | On push / PR |
| EXPLAIN gate (CI) | Seq scans, cost budget, missing indexes | On PR against seeded DB |
| Plan-regression test | Access-method or cost drift over time | On PR and nightly |
| Migration dry-run | Lock time, failures on prod-sized data | Before merge / in staging |
None of this replaces human review; it removes the toil so humans can do the part machines cannot. Pair the automation with the performance tuning fundamentals and you get a system where regressions are caught by a robot at 2am instead of by a customer at noon.
Team conventions and PR etiquette for SQL
The best review checklist is worthless if half the team ignores it and the other half applies it inconsistently. Conventions are what make review predictable, and predictable review is fast review. Write them down, keep them short, and enforce the mechanical ones with tooling so the document does not have to police itself.
- A written SQL style guide. Keyword case, alias rules, join syntax, naming (singular vs plural tables, snake_case columns, index naming). Disagreements are settled once, in the document, not per pull request.
- Every nontrivial query PR includes its EXPLAIN. Make it a template checkbox. A performance claim without a plan is an opinion, and opinions do not merge.
- Migrations are their own PR. Never bundle a risky schema change with feature code; it makes rollback all-or-nothing and hides the migration from focused review.
- Name the reviewer who owns the schema. For changes to sensitive tables (billing, auth, audit), require a review from someone who owns that domain, the same way you gate security-sensitive code.
Etiquette matters as much as rules. As an author, give reviewers the context they need: what the query is for, the row counts involved, the plan, and any tradeoff you made. As a reviewer, separate blockers from preferences explicitly, so the author knows what must change before merge versus what is a suggestion. Phrase performance concerns as questions backed by the plan ("the plan shows a seq scan on orders, is there an index we can add?") rather than verdicts. The goal of the review is a correct, fast, maintainable change and a team that is a little better at SQL than it was yesterday, not a scoreboard. The SQL developer path is largely about internalizing these habits until they are reflex.
The review checklist
Turn the seven concerns into a scannable list. Severity guides whether a finding blocks the merge or is a follow-up.
| Check | Why it matters | Severity |
|---|---|---|
| User input is parameterized, never concatenated | Prevents SQL injection, the top critical class | Blocker |
| App account has least-privilege grants | Limits blast radius of any breach or bug | Blocker |
| Multi-write logic is wrapped in a transaction | Prevents partial, money-losing states | Blocker |
| Aggregates have a known row grain (no fan-out) | Silent wrong totals from join multiplication | High |
| NULL comparisons use IS / NOT EXISTS | NOT IN and <> drop or zero out rows | High |
| Predicates are sargable (no function on column) | Keeps indexes usable at scale | High |
| No N+1 loop; EXPLAIN attached for new queries | Turns a fast page into a slow one under load | High |
| Migration is idempotent and has a rollback | Re-runs and reverts must be safe | High |
| Migration avoids long locks on large tables | Prevents an outage during deploy | High |
| Procedure states its transaction and error contract | Callers and retries depend on it | High |
| EXPLAIN gate and linter pass in CI | Automates the mechanical checks | Medium |
| No SELECT * in shipped code | Fragile, wasteful, blocks covering indexes | Medium |
| Explicit JOIN syntax, meaningful aliases | Reviewers can actually see the bug | Medium |
| Set-based instead of cursor unless justified | Performance and clarity | Medium |
Review with EXPLAIN, not opinions
Performance arguments in a review thread are unfalsifiable until someone reads the plan. "This will be slow" and "the index will handle it" are both guesses. EXPLAIN (and EXPLAIN ANALYZE where safe) turns the guess into evidence.
-- Ask the author to paste this for any nontrivial new query
EXPLAIN ANALYZE
SELECT id, email FROM users WHERE email = ?;What to look for in the output: a sequential/full-table scan on a large table where you expected an index seek, an estimated row count wildly different from reality (stale statistics), and a nested loop over a big outer set (a plan-level N+1). The table below maps common plan lines to the question they should raise.
| Plan line | Ask |
|---|---|
| Seq Scan / Full Table Scan on big table | Is there a missing or non-sargable index? |
| rows estimate far from actual | Are statistics stale? Run ANALYZE. |
| Nested Loop with large outer rows | Would a hash/merge join or batching be better? |
| Sort spilling to disk | Can an index provide the order, or raise work_mem? |
| Filter removing most rows after the scan | Move the predicate into an indexable form. |
Norm to set: for any query touching a large table, the PR should include its EXPLAIN output. This makes performance review objective and fast, and it teaches authors to read their own plans.
Where SQL defects actually cluster
Across code reviews, the defects that reach the SQL review are not evenly spread. The chart below is representative of where reviewers most often find issues, and it is a useful reminder of where to spend attention.
The lesson is not that security or transactions matter less; they are rarer but far more severe when missed. Performance and correctness are where the volume is, so they reward a systematic checklist most.
A reviewer's rubric
When you sit down with a SQL diff, work these steps in order. The first three are blockers; if any fails, the review stops there.
Is it safe?
All user input parameterized. No new broad grants. Any dynamic identifier is allow-listed. If not, request changes and stop.
Is it atomic?
Related writes share a transaction with a rollback path. Procedures re-raise on error. Deadlock-prone paths retry.
Is it correct?
NULLs handled explicitly. GROUP BY is complete. No aggregate over an un-checked join grain. Edge cases (empty set, duplicates) considered.
Will it scale?
Predicates sargable, indexes present, no SELECT * or N+1. EXPLAIN attached and sane for large tables.
Can the next person read it?
Explicit joins, meaningful aliases, comments on intent, set-based over cursors. Migrations idempotent and reversible.
A SQL review is not about catching style nits; it is about asking, methodically, what this code does when it meets real data and real concurrency. Run the rubric top to bottom, insist on EXPLAIN over opinion, and the silent defects stop being silent. If you want structured practice, our SQL developer resources and the common SQL mistakes guide are good next reads.