SQL Review

Reviewing SQL

A reviewer's leverage is what the toolchain cannot say — and in SQL that is almost everything. There is no type checker for query meaning: the parser accepts anything well-formed, and sqlfluff checks layout and naming, not correctness. A query that returns the wrong rows and a query that returns the right ones are equally valid to every tool in the pipeline.

So this skill is longer on checks and shorter on "already covered" than any other in the review category. Three themes carry most of it:

1. NULL is not a value, and three-valued logic silently changes results.
2. A query is correct only under the isolation level it actually runs at.
3. The plan, not the text, decides the cost — and small edits change plans.

The snippets are fragments cut down to the defect, not runnable programs.

Already covered - do not spend review on it

That table is short on purpose. Assume nothing checked the meaning of this query, because nothing did.

NULL and three-valued logic

NOT IN against anything nullable

SELECT * FROM orders
WHERE customer_id NOT IN (SELECT id FROM banned);   -- one NULL in `banned` returns ZERO rows

When you see it: NOT IN (subquery) where the subquery column is not declared NOT NULL.

Ask: can that column be NULL? x NOT IN (1, NULL) evaluates to UNKNOWN, not TRUE, so the row is filtered out — every row. The query returns an empty set and looks like a data problem. NOT EXISTS has the semantics people expect here, and it is also usually the better plan.

An aggregate that silently skips NULLs

SELECT AVG(rating) FROM reviews;      -- ignores NULL ratings entirely
SELECT COUNT(rating) FROM reviews;    -- not the row count

When you see it: AVG, SUM, COUNT(column) on a nullable column, especially when the result is presented as "the average" of something.

Ask: is skipping the missing values the intended definition? COUNT(*) and COUNT(col) differ exactly here, and the difference is invisible until some rows are NULL.

<> excluding rows that should match

SELECT * FROM users WHERE status <> 'banned';   -- rows with NULL status are excluded

When you see it: any inequality on a nullable column.

Ask: should NULL rows appear? Comparison with NULL is UNKNOWN, so they are dropped from both = 'banned' and <> 'banned'. That is often not what the sentence in the ticket said.

Concurrency and transactions

Read-modify-write with no lock

SELECT balance FROM accounts WHERE id = 1;          -- app subtracts
UPDATE accounts SET balance = 90 WHERE id = 1;      -- last writer wins

When you see it: a SELECT whose value is computed on and written back, across two statements.

Ask: what happens with two concurrent callers? Both read 100, both write 90, one debit disappears. Correct in every test that runs single-threaded. The fixes are SELECT ... FOR UPDATE, an atomic UPDATE ... SET balance = balance - 10, or an optimistic version column — and they differ in what they cost.

An assumption that only holds at a higher isolation level

BEGIN;
SELECT COUNT(*) FROM seats WHERE event = 1 AND free;   -- 1
-- another transaction books it here
INSERT INTO bookings VALUES (...);                     -- oversold
COMMIT;

When you see it: a check-then-act inside a transaction; any invariant enforced by reading before writing.

Ask: which isolation level does this connection use? The default is READ COMMITTED in PostgreSQL, SQL Server and Oracle, and REPEATABLE READ in MySQL — none of which prevents this. Only SERIALIZABLE does, and then the code must handle serialisation failures by retrying. A unique constraint is usually the cheaper answer.

A migration that locks a large table

ALTER TABLE orders ADD COLUMN status text NOT NULL DEFAULT 'new';
CREATE INDEX idx_orders_user ON orders(user_id);

When you see it: DDL in a migration against a table that is large in production.

Ask: how long does this hold its lock, and on what? Adding a NOT NULL column with a default rewrites the whole table on older PostgreSQL (fixed in 11) and on MySQL depending on the algorithm. CREATE INDEX blocks writes unless it is CONCURRENTLY — which in turn cannot run inside a transaction, so the migration tool has to be told.

Plans, indexes and cost

A predicate that disables the index

WHERE DATE(created_at) = '2026-01-01'      -- function on the column: no index use
WHERE user_id = '42'                       -- text compared to an integer column
WHERE name LIKE '%smith'                   -- leading wildcard

When you see it: a function, a cast, or an implicit type coercion applied to the column side of a predicate; a LIKE starting with %.

Ask: can the index still be used? Wrapping the column forces a scan of every row. The range rewrite (created_at >= '2026-01-01' AND < '2026-01-02') is index-friendly and means the same thing. Implicit coercion is the sneakiest: it works, returns the right answer, and quietly does a full scan.

SELECT * in anything durable

CREATE VIEW active_users AS SELECT * FROM users WHERE active;

When you see it: SELECT * in a view, a stored procedure, an INSERT ... SELECT, or application code that maps columns positionally.

Ask: what happens when a column is added? Views can bind their column list at creation and not see it; INSERT ... SELECT breaks or, worse, shifts values into the wrong columns. It also fetches large columns nobody reads.

Pagination with a non-deterministic order

SELECT * FROM posts ORDER BY created_at DESC LIMIT 20 OFFSET 40;

When you see it: LIMIT/OFFSET with an ORDER BY that is not unique.

Ask: are ties possible? Rows sharing created_at may come back in any order, and a different order per page — so a row can appear on two pages or on none. Adding a unique tiebreaker (, id DESC) fixes correctness; keyset pagination also fixes the cost, since OFFSET still reads and discards the skipped rows.

A join that multiplies rows before aggregating

SELECT o.id, SUM(p.amount)
FROM orders o
JOIN payments p ON p.order_id = o.id
JOIN tags t ON t.order_id = o.id      -- second one-to-many: SUM is now inflated
GROUP BY o.id;

When you see it: two or more joins to one-to-many relations in the same query, with an aggregate over one of them.

Ask: does either join fan out? Each payment row is duplicated once per tag, so the total is multiplied. The result is plausible, never zero, and wrong. Aggregate in a subquery or use FILTER/DISTINCT deliberately.

An ORM query issuing one statement per row

-- the diff shows one query; the log shows 501
SELECT * FROM orders;                          -- then, per order:
SELECT * FROM customers WHERE id = ?;

When you see it: a loop in application code touching a lazy relation, or a serialiser walking associations.

Ask: how many statements does this make for N rows? The SQL in the diff is fine; the defect is at the boundary, which is why it survives every SQL-level check. Eager loading or a join fixes it.

String-built SQL

-- app side: "SELECT * FROM t WHERE name = '" + input + "'"

When you see it: concatenation or interpolation building a statement from anything not a literal — including an "internal" value, an ORDER BY column name, or a table name.

Ask: is every interpolated part a bound parameter? Parameters cover values and cannot cover identifiers, so a dynamic column or table name needs an allowlist rather than escaping. This is the one SQL finding some tools do catch — on the application side, not here.

Engine-dependent - establish which database before commenting

Almost nothing in SQL is portable at the level that matters for review.

What to say

Anchor the comment to the statement, name the condition that triggers the defect, and say what breaks: "banned.id is nullable, so a single NULL there makes this NOT IN return no rows at all" beats "prefer NOT EXISTS". If you cannot state the data that produces the wrong answer, it is a preference, not a defect.

Separate correctness from cost, and say which you are raising. A wrong result and a slow query deserve different urgency, and mixing them in one comment makes both easier to dismiss.