Code Review and Working with Existing Codebases Questions

Direct answer

A rollback-safe rename plus backfill never touches the old column or existing readers directly. It adds the new column alongside the old one, writes to both while backfilling the new one in batches, verifies the backfilled data matches, only then switches reads over behind a flag, and keeps the old column around for a retention window so any step can be reversed just by flipping the flag back, not by undoing a destructive change.

Structured elaboration

Each phase below names what code review should specifically confirm before approving it, as a "Review check," plus what to monitor once it ships.

1. Schema change. Add the new column as nullable, with no constraints yet:

ALTER TABLE events ADD COLUMN new_name text NULL;

Review check: confirm this specific statement is additive only and backward-compatible, meaning every existing reader and writer keeps working unmodified the moment this ships, with zero application changes required yet.

2. Dual-write. Deploy an application change, behind a feature flag, that writes both the old and new column on every write to a row. Review check: is the write to both columns transactional or otherwise guaranteed consistent (not "write old, then separately and non-atomically write new"), and is the flag off by default so this ships dormant before anything depends on it?

3. Backfill. A batched, idempotent job fills in the new column for existing rows, only where it's still NULL, ordered by primary key, with a checkpoint so it can resume after an interruption instead of restarting from row one:

UPDATE events
SET new_name = old_name
WHERE id BETWEEN :batch_start AND :batch_end
  AND new_name IS NULL;

Review check: is progress persisted somewhere durable (not just in the running process's memory), and does re-running an already-completed batch do nothing (true idempotence), not create incorrect data? Monitoring: track batches completed, rows backfilled, replication lag, and write error rate on the table for the duration of the backfill, and pause automatically if replication lag crosses an agreed threshold.

4. Verification. Before trusting the backfill, sample a random set of rows and confirm new_name matches what old_name implies for each. Review check: is the sample size and comparison method actually specified in the PR, not just asserted as "we verified it"?

5. Read cutover. Only after verification passes, flip the flag so reads prefer the new column, falling back to the old one if the new one is somehow still empty for a given row. Review check: is the fallback logic actually tested, not just written? Monitoring: application error rate and query latency on this table specifically, right after the flag flips, since a regression here is the trigger for the rollback shown in the diagram below.

6. Cleanup. Once reads have run on the new column successfully for a defined retention window, make it NOT NULL, add any index it needs, and only then drop the old column, in a separate, later PR. Review check: is dropping the old column genuinely a separate step from everything above, so it can never accidentally ship bundled with a change that hasn't been verified yet?

flowchart LR
  A[Step 1: add new_name column, nullable] --> B[Step 2: dual write old_name plus new_name]
  B --> C[Step 3: backfill new_name in batches where NULL]
  C --> D{Verification: sampled row values match}
  D -- mismatch found --> C
  D -- fully verified --> E[Step 4: flip reads to new_name behind a flag]
  E --> F{Error rate normal after cutover}
  F -- regression --> G[Rollback: flip flag back to old_name, dual write stays intact]
  F -- healthy --> H[Step 5: make new_name NOT NULL, add index concurrently]
  H --> I[Step 6: drop old_name after a retention window]

Worked example

Renaming user_email to primary_email on a table with 40 million rows, backfilled in batches of 5,000 rows: that's 40,000,000 / 5,000 = 8,000 batches total. With a short pause between batches to keep replication lag bounded, the job runs as a background process over however long it takes to work through all 8,000 batches, checkpointing its position after each one so a restart resumes from the last completed batch instead of row one. Verification samples 10,000 random rows after the backfill reports complete and confirms primary_email equals user_email for every one of them before the flag is ever flipped to prefer reads from the new column.

Trade-offs and pitfalls

Every step here is reversible specifically because the old column and old read path stay intact until the very last, separate cleanup step, which is exactly what makes this slower and more code than a single rename statement; that trade is worth it for a heavily-used column and wrong for a rarely-touched internal table, where a single migration with a maintenance window might be simpler and perfectly safe. The most dangerous version of this pattern to review is one that quietly combines two of these steps, most often shipping the read cutover and the old-column drop in the same change, which collapses the rollback safety the whole design exists to provide.

Direct answer

Infrastructure code (Terraform, Ansible, Bash, and Python scripts that provision or configure systems) gets reviewed against the same bar as application code, plus more weight on blast radius (how much of the system, or how many users, a bug here could damage before anyone catches it), secrets, and rollback, because a bug here can take down a live environment or leak a credential, and there's rarely a staging replica that catches it first.

Structured elaboration

Worked example

A PR adds a Terraform module that provisions an S3 bucket for application logs. Correctness: the plan shows a new bucket being created, no destroy. Security: the bucket policy is checked for public read access left open by default, flagged and narrowed. Testability: terraform validate and a linter both run in CI. Operational readiness: the PR description names the CloudWatch metric to check post-apply and states the rollback is terraform destroy on this specific module since nothing else depends on it yet. Documentation: a comment explains why a 90-day lifecycle expiration was chosen. Secret handling: no credentials appear in the diff; the module references an existing secret manager entry.

Trade-offs and pitfalls

Applying every item on this checklist to a one-line change to a dev-only resource is overkill; scale depth to blast radius, not to the existence of the checklist. The most common wrong turn is approving because the Terraform plan "looks clean" without actually reading whether a resource will be destroyed and recreated, which is the single most frequent way an infra review misses an avoidable outage.

Direct answer

On a 25-file pull request (PR, a proposed code change submitted for review), I triage by actual risk and blast radius, not by diff size: the provisioning-script bug and the Terraform security misconfiguration are must-fix because they change behavior or expose risk, most naming or structure issues are should-fix depending on whether they'll cost real time later, and pure style preferences that a linter could enforce are optional or shouldn't be a manual comment at all.

Structured elaboration

How I classify

• Must-fix before merge: anything factually wrong, or that causes an outage or security exposure, or violates a hard team rule. If I can point to a concrete failure scenario, it's must-fix.
• Should-fix before merge: real quality issues that won't break anything today but will cost real time later (unclear naming, missing error handling on a path likely to be hit, no test for new logic). Negotiable in a specific stated case, but the default is fix it.
• Optional: preference, style, or a nice-to-have that doesn't change correctness or meaningfully affect future maintainability. If a linter or formatter could enforce it, it shouldn't be a manual comment at all.

Two example comments per category

Must-fix

1. "The provisioning script writes the instance ID to a temp file before checking whether the previous run's file still exists, so a retried run silently appends stale data instead of failing loudly. This will misconfigure real instances on retry." Justification: a concrete correctness bug with an observable, harmful production effect.
2. "This Terraform resource sets the security group's inbound rule to allow all internet addresses on port 22 (SSH, the protocol used for remote server access). This needs to be scoped to the internal network's address range before merge." Justification: an active security exposure, not a style question; shipping it creates real risk the moment it's applied.

Should-fix

1. "This function handles three unrelated things: validation, provisioning, and notification. Pulling the notification piece out would make this testable in isolation and easier for the next person to change on its own." Justification: doesn't break anything today, but the coupling will slow down every future change to this function.
2. "There's no test covering the retry path this PR adds. Given the must-fix bug above lived exactly in that retry path, a test here would have caught it." Justification: directly tied to the risk just found, not a generic "add more tests" comment.

Optional

1. "nit: could use an f-string here instead of string concatenation, purely a style preference, not blocking." Justification: no functional or long-term readability cost either way, and explicitly labeled non-blocking so the author knows they can skip it.
2. "nit: consider renaming the loop variable for clarity, but it's also readable as-is from context." Justification: minor and defensible either way, flagged as optional rather than demanded.

Worked example

Given all three issue types in the same PR, I'd leave the two must-fix comments first, clearly marked (a "BLOCKING:" prefix, or the review tool's "request changes" status), and ask for a re-review specifically on those two before anything else. The should-fix comments go in the same review round but don't need a second look; I'd trust the author to either fix them or explicitly push back with a reason. The optional style comments get grouped at the bottom, or left as a single batch of nits, so they don't compete visually with the two things that actually block merge, and I say explicitly that the PR is approvable once the two blocking items are addressed, regardless of whether the nits are touched.

Trade-offs and pitfalls

• Leaving 15 style nits with the same visual weight as the security misconfiguration buries the one comment that actually matters; always lead with, and visually separate, the must-fix items
• Treating "should-fix" as a synonym for "must-fix" just because you feel strongly about it erodes the whole point of the tiering; if it isn't tied to a concrete failure or real future cost, it isn't must-fix
• Style-only nits that a formatter or linter could catch shouldn't be manual review comments at all, that's a signal to add the check to continuous integration (CI, the automated build/test pipeline) instead of repeating the same comment on every PR

Direct answer

None of the three tasks here is idempotent, meaning running the playbook a second time against a server that's already correctly configured should report no changes, but this one still reports changes, or worse, causes them, every single time. Each task needs to move from an imperative shell command to a declarative module that checks the current state before acting, and the fix should be validated by actually running the playbook twice and confirming the second run reports zero changes, not just by reading the code and assuming it's fine.

Structured elaboration

Task 1: command: apt-get install -y nginx. A raw shell command has no idea whether nginx is already installed; it just runs apt-get install every single time. That might be a no-op at the package-manager level, but Ansible itself has no way to know that and will always report this task as "changed," which defeats the entire point of using a configuration-management tool.

Task 2: the copy task for the config file. This one is closer to idempotent already, since Ansible's copy module compares the destination file's content against what's being written and only reports a change when the content actually differs. It's still incomplete though: no explicit file permissions or owner are set, and a config change should trigger a service reload, not happen silently with no connection to the next task.

Task 3: service: state: restarted. This always restarts the service on every single run, whether or not anything actually changed. It's the least idempotent line in the whole playbook: running this playbook nightly, for example on a schedule, would bounce nginx nightly for no reason at all.

The fix. Use Ansible's notify/handler pattern: the config-file task notifies a handler, and the handler, which reloads or restarts the service, only runs when that specific task actually reported a change. A no-op run then touches the service zero times.

Worked example

A corrected version of the playbook:

- hosts: web
  become: true
  tasks:
    - name: install nginx
      apt:
        name: nginx
        state: present

    - name: place nginx site config
      copy:
        content: "server { listen 80; }"
        dest: /etc/nginx/sites-available/default
        owner: root
        group: root
        mode: '0644'
      notify: reload nginx

  handlers:
    - name: reload nginx
      service:
        name: nginx
        state: reloaded

Why each change matters: the apt module is declarative, it checks the package's actual state first, so a rerun is cheap and honest about whether anything changed. notify plus a handler means the service only restarts, specifically via reloaded, which is less disruptive than a full restarted, exactly when the configuration actually changed, not on every run regardless of state.

How to test this in CI (continuous integration). Use Molecule, a testing framework built specifically for Ansible roles, to spin up an ephemeral container and converge (run) the playbook against it, then converge a SECOND time and assert the second run reports zero changed tasks, that's a direct, mechanical test of "is this actually idempotent," rather than trusting it by inspection. Add a verify step, using a tool like Testinfra, asserting the real end state: nginx is installed, the config file has the expected content, and the service is running. Wire this into the CI pipeline so a role that regresses on idempotency fails the build automatically, instead of being caught by a human rerunning it by hand much later.

Trade-offs and pitfalls

reloaded is gentler than restarted, but not every application supports a clean reload; some genuinely need a full restart to pick up certain kinds of configuration changes, so this substitution has to match how the real service actually behaves, not be applied blindly to every service task. Testing idempotency by running the playbook twice in CI adds real time to every pipeline run, a fair cost for something this cheap to verify and this easy to silently break without anyone noticing.

Direct answer

For a migration touching a 5GB, heavy-write table, I check six things specifically: whether the change runs inside a transaction and what that actually implies for locking, how long any lock is held and what kind, whether backfills are batched instead of one giant statement, how the change rolls out (all at once or staged), what's monitored during and after, and whether there's a real rollback path, not just "revert the migration file."

Structured elaboration

• Transactional behavior. Some DDL (data definition language, schema-changing statements like ALTER TABLE) is transactional and can be rolled back cleanly if something fails mid-statement; some isn't, and a failure partway through leaves the schema in a partially-changed state. This differs by database engine and even by the specific kind of ALTER, so I check what's actually true for the database and statement in question rather than assuming.
• Lock duration. The real question is what kind of lock the statement takes and for how long. On Postgres, ADD COLUMN with a constant default has been a fast, metadata-only change since Postgres 11, but adding a NOT NULL constraint that requires validating every existing row, or an index build without CONCURRENTLY, still takes a lock for the duration of that work. On MySQL 8.0+, many ALGORITHM=INSTANT operations (a modifier that changes only table metadata instead of rewriting existing rows, including some column adds) are near-instant, but not all ALTER variants qualify, and older MySQL versions may rebuild the whole table. I check which case this migration actually falls into for the target database and version, not just assume it's cheap.
• Backfills. A backfill on a 5GB table should never be one giant UPDATE, which holds locks and generates a huge amount of write-ahead log or binlog (the record every database write generates so changes can be replayed or recovered) in one shot. It should run in small batches (a bounded WHERE clause, a sleep or throttle between batches), be resumable from where it left off, and be idempotent (safe to re-run a batch that partially completed).
• Rollout strategy. For anything beyond a trivial, fast schema change, I look for a staged rollout: add new structure first without changing behavior, deploy application code that can handle both old and new shapes, backfill, then only later switch behavior over, rather than one migration that changes schema and behavior simultaneously.
• Monitoring. During the migration I want to see replication lag, lock wait time, and query latency on this table specifically, not just overall database health, since a table-specific problem can hide inside a healthy-looking aggregate.
• Rollback. A real rollback plan says what to actually do if something goes wrong mid-migration, not just "we have backups." For a pure schema addition, that's usually just dropping the new column; for anything involving a backfill or application-visible behavior change, it needs to say how to safely stop and either resume or revert without leaving the table in a half-migrated state.

Worked example

The migration adds a NOT NULL column with a default to the 5GB table. On a database and version where that's not a metadata-only operation, the safe sequence is: add the column as nullable with no default first (cheap), backfill it in batches ordered by primary key, verify every row is populated, then add the NOT NULL constraint (which on Postgres can validate against already-known-good data faster once nothing is actually null). A batch looks like:

UPDATE big_table
SET new_col = <derived value>
WHERE id BETWEEN :batch_start AND :batch_end
  AND new_col IS NULL;

run in a loop over consecutive primary-key ranges with a short pause between batches, so the migration never holds a lock or generates a burst of writes for longer than one small batch takes.

Trade-offs and pitfalls

Splitting a migration into more, smaller, safer steps is real added engineering and coordination work compared to "just run the ALTER," and for a genuinely small or low-traffic table that overhead isn't justified; the 5GB, heavy-write detail in the question is exactly what tips the balance toward doing it the careful way. A common pitfall is testing the migration only against a small local database, where an operation that's instant on a toy table can be a multi-minute, lock-holding operation on the real, much larger one, so the review should ask whether this was tested against production-scale data, not just correctness-tested against a handful of rows.