Reviewing database changes

Database changes are the highest-risk work in this project. They are applied by hand, they are not covered by any test, they often cannot be undone, and the data they touch belongs to real people.

This page describes how to make one safely. It exists because two migrations in this repository were wrong in ways that review did not catch, and were found only by running them first.

Test against a copy before production

The rule: never let a nontrivial migration meet production data for the first time.

Make a throwaway database, load the real schema and a copy of the data into it, run the migration there, and check the result with queries. This takes a few minutes and has already caught two bugs that would have caused silent data corruption.

What counts as nontrivial is anything that modifies existing rows, anything involving a trigger, anything that adds a constraint to a column that already holds data, and anything that drops or recreates a policy.

Adding a nullable column is trivial. Almost nothing else is.

Check what your change will actually fire

The most subtle failure in this repository came from a migration that repaired rows with a loop of updates, without realising that each update fired the table's update trigger. The trigger saw that the batch year had not changed, took its "preserve the existing identifier" branch, and reset each row straight back to its previous value. The migration reported success. Nothing had changed.

Before writing a repair, ask what triggers exist on the table and what they will do to your writes. If a trigger will fight the repair, disable it around the repair and re-enable it in the same transaction.

Check that your filter matches where the data lives

The second bug: an allocator looked for the highest existing identifier by filtering on a column, while the value it needed was embedded in a different column. As soon as the two disagreed for any row, the allocator stopped seeing that row and reissued its number, creating a duplicate.

When a query decides what value to write next, be certain it can see every row that already holds one.

Do not remove a constraint to make an error go away

When duplicate identifiers first appeared, the unique constraint was dropped. The error stopped. The duplicates continued, now silently, and one identifier ended up on two live profiles.

A constraint failing is the constraint working. Fix what is producing the bad value.

Policies

When adding or changing a policy:

• An update policy needs both USING and WITH CHECK. Omitting the second is a vulnerability, not a style choice. See How access control works.
• New tables should revoke the default grants explicitly and enable FORCE ROW LEVEL SECURITY, rather than relying on policies alone.
• A new function needs REVOKE ALL ... FROM PUBLIC, because creating one grants execute to everyone by default.
• A SECURITY DEFINER function should set an empty search path and fully qualify every name it references.

Remember that a file which drops and recreates policies is dangerous if it fails halfway: the table is left with row-level security enabled and no policy, which denies everyone. Since each file runs as a single transaction, a failure rolls back, but only if you have not split the work across files unnecessarily.

Enum values need two files

PostgreSQL will not allow a new enum label to be used in the transaction that adds it. Adding a value and writing a policy that references it must be two migrations, applied in order, the first committed before the second runs. Say so in a comment at the top of both files.

Concurrency

If a migration or function computes a value from existing rows and then writes it, two callers can compute the same value simultaneously. Take an advisory lock before reading, as the identifier allocator and the role-change function both do.

This is not theoretical: two people signing up in the same second is exactly the case that produced duplicate identifiers.

What to include in the pull request

• The migration file, with comments explaining why.
• A description of what you tested it against, and what you checked afterwards.
• Whether it is reversible, and if so, how.
• Whether it needs anything done by hand first. Storage buckets, for instance, are not created by migrations, so a migration that adds policies for a new bucket depends on someone creating that bucket beforehand.

After it merges

Merging does not apply it. An owner runs the file against production as a separate step, and should confirm on the pull request once they have. Until then, any application code that depends on the change is broken in production, which is a good reason to ship the migration before the code that needs it.