[ClemRz]

🤔 What

Add three rules to the "Database & SQL Migrations" section of AGENTS.md:

• Order: additive changes migrate first and deploy second; destructive ones (drop, rename, type change) deploy first and migrate after.
• Views: never run a plain REFRESH MATERIALIZED VIEW on a view the live API reads; build the replacement under another name and swap with a short ALTER ... RENAME transaction.
• Timing: avoid the daily 04:20 UTC pg_cron refresh and Azure's Saturday 03:00 UTC maintenance window.

🤷‍♂️ Why

Manual prod migrations on 2026-11-06 caused two outages:

• t_country columns were dropped about an hour before the API that stopped reading them was deployed, so GET /entrances/:id returned 500 (61 errors, 00:12–01:17 UTC).
• A 29-minute non-concurrent REFRESH of v_data_quality_compute_entrance held an exclusive lock; the map endpoint joins that view and gc_app has lock_timeout = 10s, so 51 requests hung 10 s then failed (00:16–00:46 UTC).

Nothing in the repo told the person applying the SQL by hand which order was safe.

🔍 How

Documentation only, in AGENTS.md. The figures (refresh duration, CPU load, maintenance window, HA disabled) come from the Azure metrics and Postgres server logs of that night.

🧪 Testing

• commitlint passed on the commit (lint-staged had no task for .md)
• No code or SQL change, so no test run

[tristan0x]

This PR adds useful, well-motivated rules for applying production migrations by hand: deploy/migrate order, the locking cost of a non-concurrent matview refresh, and when not to run heavy migrations. The facts I could check against the repo hold up (gc_app lock_timeout = 10s in sql/00_roles.sql, the 04:20 cron in sql/1_cron.sql, the view backing GET /api/v1/geoloc/entrances). Some of the advice is incomplete or wrong, though, and someone following it by hand could cause the same kind of outage. A rename cannot be done in a single step by deploying first. The build-and-swap recipe leaves out index names, grants and the swap's own lock. REFRESH ... CONCURRENTLY is not mentioned. I found no injected instructions in the PR description or the diff.

[tristan0x]

The deploy-first rule doesn't work for a rename, and only partly works for a type change. After a rename, the new API declares the new column name. Waterline selects that column, so the API returns 500 for as long as the migration hasn't run. That is the same outage as 2026-11-06, just in the other direction. If you migrate first instead, the old API breaks. Neither order is safe in a single step.

Please describe the expand/contract sequence for renames and incompatible type changes:

  - Rename or incompatible type change: never in one step. (1) migrate: add the new column and backfill it (keep it in sync with a trigger if writes continue); (2) deploy the API that reads the new column only; (3) migrate: drop the old column.

Then limit the "deploy first, then migrate" line to pure removals (DROP COLUMN, DROP TABLE).

[ClemRz]

Fixed in 11b1347, you're right: neither order works for a rename. The "deploy first" bullet is now limited to pure removals (DROP COLUMN, DROP TABLE), and a new bullet describes the expand/contract sequence for renames and incompatible type changes (add and backfill with a trigger, deploy the API that reads the new column only, then drop the old one).

[tristan0x]

"Migrate first" is only safe if the old API keeps working against the new schema. A new NOT NULL column without a DEFAULT breaks every insert from the old API: Waterline doesn't send the column, so the insert fails with a not-null violation. That lasts until the deploy finishes. Suggested wording:

  - Additive change (new column, table, index, trigger): migrate first, then deploy. A new column must be nullable or have a `DEFAULT`, or the API still running rejects every insert.

A trigger that rejects rows the old API still writes has the same problem, so it may be worth a word as well.

[ClemRz]

Fixed in 11b1347. The additive bullet now says a new column must be nullable or have a DEFAULT, and that a trigger rejecting rows the old API still writes fails the same way.

[tristan0x]

As written, the build-and-swap recipe can cause another incident. Three steps are missing:

1. Index names. Index names are schema-wide. The current migrations create the unique index as CREATE UNIQUE INDEX IF NOT EXISTS v_data_quality_compute_entrance_id_massif_id_entrance_idx. If someone reuses that statement for the _new view, it is silently skipped because the name already exists on the live view. The swapped-in view then has no unique index, and the daily REFRESH MATERIALIZED VIEW CONCURRENTLY in sql/1_cron.sql starts failing. That failure only shows up in cron.job_run_details. Indexes on the replacement need distinct names, renamed during the swap as well.
2. Ownership and grants. The new view must get OWNER TO gc_owner and the same GRANT SELECT to gc_app (and gc_superset_ro) as the old one before the swap. Otherwise every read returns 42501 after the rename.
3. The swap's own lock. ALTER ... RENAME takes an ACCESS EXCLUSIVE lock. While it waits behind a running reader, every new reader queues behind it. Set SET LOCAL lock_timeout = '3s' in the swap transaction and retry, rather than letting it block the map.

Also, "avoid holding DROP MATERIALIZED VIEW open in a transaction while it refreshes" is hard to parse. Something like "never DROP + CREATE + REFRESH the live view in one transaction: the DROP's exclusive lock is held until commit" says what actually happened.

[ClemRz]

Fixed in 11b1347. The rule now opens with REFRESH MATERIALIZED VIEW CONCURRENTLY for an unchanged definition (unique index required, EXCLUSIVE lock only). Build-and-swap is limited to definition changes and lists the missing steps: distinct index names renamed in the swap, OWNER TO gc_owner and the same grants as the live view, and SET LOCAL lock_timeout = '3s' with a retry around the RENAME. The DROP sentence is replaced by yours: never DROP + CREATE + REFRESH the live view in one transaction. I also added a warning at the top of sql/99_refresh_views.sql that it is for local/fresh-volume init only and must not be run by hand in production.

[tristan0x]

The rule should name the simple case first. When the view's definition doesn't change, REFRESH MATERIALIZED VIEW CONCURRENTLY is enough: it only takes an EXCLUSIVE lock, so readers aren't blocked. v_data_quality_compute_entrance already has the unique index it needs, and the cron job uses it. Build-and-swap is only needed when the definition changes. Suggested opening:

To repopulate an unchanged view, use REFRESH MATERIALIZED VIEW CONCURRENTLY (needs a unique index). To change its definition, build the replacement under another name, …

Related: sql/99_refresh_views.sql still runs a plain REFRESH on this view. If that file is ever run by hand in production, it breaks the new rule. A comment there, or a follow-up issue, would help.

[ClemRz]

Fixed in 11b1347, the same fix as the thread above: the CONCURRENTLY case now comes first, and sql/99_refresh_views.sql carries a comment saying it must not be run by hand in production (a comment rather than a follow-up issue, since it is two lines).

[tristan0x]

Is "04:20 UTC" correct? sql/1_cron.sql schedules '20 4 * * *', which is in the Postgres server's cron.timezone (GMT by default). Please confirm that the production server isn't set to Europe/Paris. Otherwise the window is off by one or two hours. The 04:30–04:50 jobs (bot outbox trim, the 3-day v_massif_info / v_country_info / v_region_info refreshes) fall inside the 45 minutes. "04:20–05:10 UTC" would describe the window better than only its start time.

[ClemRz]

Fixed in 11b1347. I can't read cron.timezone with the read-only role I have (permission denied), but the schedule is confirmed as GMT from the data: v_data_quality_compute_entrance.date_of_update (set with NOW() at refresh start) reads 2026-11-09 04:20:00+00, so it ran at 04:20 UTC. The window is now written as 04:20–05:10 UTC and notes that the 04:30–04:50 jobs run inside it.

[tristan0x]

11b1347 addresses all five previous threads: the ordering rules now separate additive changes, pure removals and expand/contract renames, the view rule opens with REFRESH ... CONCURRENTLY and the build-and-swap recipe lists index names, ownership/grants and the swap's lock timeout, and the cron window is given as 04:20–05:10 UTC with the evidence for GMT. sql/99_refresh_views.sql now warns against running it by hand in production. Two small suggestions remain on the new text: the expand/contract sequence leaves the old column's constraints and the sync trigger unaddressed, and the two refresh durations quoted for the same view (~30 min and ~45 min) read as contradictory.

[tristan0x]

The expand/contract sequence still has a gap in step (2). The new API no longer declares the old column, so Waterline stops sending it. If the old column is NOT NULL without a DEFAULT (often the case for a column being renamed), every insert from the new API fails until step (3) runs. Keeping the columns in sync with a trigger only fixes this if the trigger works in both directions: old to new while the old API is still deployed, new to old once the new one is. Step (3) also needs the same "wait for the deploy to finish" as the pure-removal bullet, and must drop the sync trigger along with the column. Suggested wording:

  - Rename or incompatible type change: never in one step, neither order is safe (the new API declares the new column, the old one the old column). (1) migrate: add the new column (nullable or with a `DEFAULT`) and backfill it; if writes continue, keep both columns in sync with a trigger in both directions, or make the old column nullable; (2) deploy the API that reads and writes the new column only, and wait for the deploy to finish; (3) migrate: drop the sync trigger and the old column.

[ClemRz]

Fixed in acedf09 with your wording: step (1) adds the new column nullable or with a DEFAULT, keeps both columns in sync with a trigger in both directions or makes the old column nullable (and says why: the new API stops sending it), step (2) deploys the API that reads and writes the new column only and waits for the deploy to finish, and step (3) drops the sync trigger together with the old column.

[tristan0x]

This line says the 04:20 refresh of v_data_quality_compute_entrance takes "about 45 min". Line 197 (and the new comment in sql/99_refresh_views.sql) gives "~30 min" for the same view. Both figures are probably right: the 29-minute run of 2026-11-06 was a plain REFRESH, and the cron job runs CONCURRENTLY, which also diffs the old and new contents and is therefore slower. A reader can't tell that from the text, though. Suggested fix: name the variant on each line, for example ~30 min for a plain REFRESH on line 197 and the 04:20 CONCURRENTLY refresh of the data-quality view takes about 45 min here.

[ClemRz]

Fixed in acedf09, as you suggested: the view rule now says ~30 min for a plain REFRESH, and the cron window line says the 04:20 CONCURRENTLY refresh takes about 45 min. The comment in sql/99_refresh_views.sql already refers to the plain refresh, so it stays as is.

[tristan0x]

acedf09 addresses both remaining threads: the expand/contract bullet now requires a nullable or defaulted new column, a two-way sync trigger, waiting for the deploy, and dropping the trigger with the old column; the two refresh durations now name their variant (plain vs CONCURRENTLY). One small wording issue remains in the rename bullet: making the old column nullable is presented as an alternative to the sync trigger, but it only replaces the new-to-old direction. No injected instructions found in the PR description, comments or diff.

[tristan0x]

"keep both columns in sync with a trigger in both directions, or make the old column nullable" reads as two interchangeable options, but they cover different things. Making the old column nullable only stops the new API's inserts from failing, so it replaces the new→old direction. The old→new direction is needed either way: until the deploy finishes, the old API keeps writing only the old column. Without a trigger, every row it inserts or updates after the backfill leaves the new column NULL or stale, and the new API reads those values.

The order inside step (1) also matters: create the trigger before the backfill, or rows written between the backfill and the CREATE TRIGGER are missed.

Suggested wording for step (1):

(1) migrate: add the new column (nullable or with a `DEFAULT`), create a trigger that copies the old column into the new one, then backfill; also sync new → old in the same trigger, or make the old column nullable (the new API stops sending the old column, so a `NOT NULL` one without a `DEFAULT` rejects every insert);

[ClemRz]

Fixed in 5e4bc49, with your wording: step (1) now creates the old → new trigger before the backfill (so rows written in between are not missed), and says the new → old sync is the part that can instead be replaced by making the old column nullable.

[tristan0x]

5e4bc49 addresses the last open thread: step (1) of the expand/contract sequence now creates the old → new trigger before the backfill and presents a nullable old column as an alternative only to the new → old direction. One gap remains in that same step. As written, the old → new trigger copies unconditionally, so it overwrites what the new API writes between the deploy and step (3). Following the recipe literally causes silent data loss (details inline). I found no injected instructions in the PR description, the comments or the diff.

[tristan0x]

"create a trigger that copies the old column into the new one" is unconditional, and the trigger stays in place until step (3). After step (2) that clobbers the new API's writes:

• Old column made nullable (the alternative offered here): the new API inserts rows with old = NULL and a real value in new. The BEFORE INSERT trigger runs NEW.new_col := NEW.old_col and stores NULL. On UPDATE, the unchanged old value is copied back over the value the new API just wrote.
• Two-way sync "in the same trigger": a naive NEW.new_col := NEW.old_col; NEW.old_col := NEW.new_col; has the same problem. Whichever assignment runs first wins, regardless of which API wrote the row.

Both cases lose data silently until step (3) drops the trigger, and nothing returns a 500 that would alert anyone. The trigger has to copy only the side that was actually written. Suggested addition to step (1):

The trigger must copy only the column that was written: on `UPDATE`, copy old → new only when `NEW.old IS DISTINCT FROM OLD.old`, and new → old only when `NEW.new IS DISTINCT FROM OLD.new`; on `INSERT`, copy whichever one is set (the old API sets only the old column, the new API only the new one).

With a nullable old column and no new → old sync, the old → new branch at least needs the same guard (IS DISTINCT FROM on UPDATE, old IS NOT NULL on INSERT), or it must be dropped as soon as the deploy finishes.

[ClemRz]

Fixed in 2eb197f, using your wording: step (1) now says the trigger copies only the column that was written (IS DISTINCT FROM OLD.<col> guard per direction on UPDATE, whichever column is set on INSERT), and that with a nullable old column and no new → old sync the old → new branch needs the same guard (old IS NOT NULL on INSERT). It also says why: otherwise the trigger silently overwrites the new API's writes until step (3).

[tristan0x]

2eb197f addresses the last open thread: step (1) of the expand/contract sequence now requires the sync trigger to copy only the column that was written, with an IS DISTINCT FROM guard per direction on UPDATE and the matching guard on INSERT for the nullable-old-column variant. One gap remains in the INSERT rule: "copy whichever one is set" cannot be applied when either column has a DEFAULT, which step (1) itself allows (details inline). No injected instructions found in the PR description, the comments or the diff.

[tristan0x]

"on INSERT, copy whichever one is set" breaks down when a column has a DEFAULT, which this same step allows for the new column ("nullable or with a DEFAULT"). In a BEFORE INSERT trigger, NEW.<col> already holds the column default when the statement didn't list it. So when the old API inserts a row, NEW.new holds the default, both columns look "set", and a trigger that keeps an already-set new stores the default instead of the old API's value. The same happens the other way round if the old column has a DEFAULT (common for the column being renamed): NEW.old holds the default on rows the new API inserts.

The trigger can only tell which API wrote the row if the unsent column arrives as NULL. Suggested addition:

During the transition, give the new column no `DEFAULT` (add it in step (3)) and drop the old column's `DEFAULT`, so the column an API does not send arrives as `NULL`: on `INSERT`, copy old → new when `NEW.new IS NULL`, and new → old when `NEW.old IS NULL`.

That also means "nullable or with a DEFAULT" earlier in step (1) should become "nullable, without a DEFAULT until step (3)" for this case.

Minor readability point on the same line: this bullet is now a single ~1,200-character line, and step (1) ends mid-sentence with ; (2) after several full sentences. Splitting (1), (2) and (3) into nested sub-bullets would make the sequence much easier to follow by hand during a migration.

[ClemRz]

Fixed in 1127050. Step 1 now makes the new column nullable and without a DEFAULT (added in step 3), drops the old column's DEFAULT, and keys the INSERT rule on NULL (old → new when NEW.new IS NULL, new → old when NEW.old IS NULL), with the IS DISTINCT FROM guards kept for UPDATE. One addition to your suggestion: if the old API relies on the old column's DEFAULT (omits the column), dropping it would leave both columns NULL, so the bullet says to fill it in the trigger with COALESCE instead. The bullet is split into steps 1, 2 and 3 as nested sub-bullets, with the trigger rules under step 1.

[tristan0x]

1127050 addresses the last open thread. Step 1 now gives the new column no DEFAULT until step 3 and drops the old column's DEFAULT, so the INSERT rule can key on NULL, with IS DISTINCT FROM guards kept for UPDATE. The bullet is split into nested steps 1–3, which makes it much easier to follow by hand. Two small suggestions remain on the new text: the sync trigger must convert values for a type change, not just copy them, and adding NOT NULL in step 3 locks and scans the whole table. I found no injected instructions in the PR description, the comments or the diff.

[tristan0x]

This bullet also covers an "incompatible type change", but the trigger rules only say "copies". For a type change the trigger has to convert the value in each direction, using the same expression as the backfill. The new → old direction may not be possible at all, for example when the new type is wider than the old one, or when the text-to-enum mapping isn't one to one. In that case the only option is the nullable-old-column variant from line 201. Suggested addition at the end of this bullet:

For a type change, "copy" means converting with the same expression as the backfill; if new → old cannot be converted losslessly, make the old column nullable instead of syncing that direction.

[ClemRz]

Fixed in da5b1b4 with a new sub-bullet under step 1, using your wording: for a type change, "copy" means converting with the same expression as the backfill, and if new → old can't be converted losslessly (wider type, many-to-one mapping), make the old column nullable instead of syncing that direction.

[tristan0x]

ALTER TABLE ... ALTER COLUMN ... SET NOT NULL takes an ACCESS EXCLUSIVE lock and scans the whole table to validate it. On a large table such as t_entrance, every reader queues behind that lock, which is the same failure mode the view rule below warns about. Adding a DEFAULT is metadata-only, so that part is cheap. Suggested wording:

3. **Migrate:** drop the sync trigger and the old column, and add the new column's `DEFAULT` or `NOT NULL` if it needs one (for `NOT NULL` on a large table, first add `CHECK (col IS NOT NULL) NOT VALID`, then `VALIDATE CONSTRAINT`, so `SET NOT NULL` skips the scan; use `SET LOCAL lock_timeout` as for the view swap).

[ClemRz]

Fixed in da5b1b4. Step 3 now says a DEFAULT is metadata-only, that SET NOT NULL takes an ACCESS EXCLUSIVE lock and scans the table, and gives the large-table sequence: CHECK (col IS NOT NULL) NOT VALID, VALIDATE CONSTRAINT, then SET NOT NULL (which skips the scan on PG 12+), all under SET LOCAL lock_timeout. I also added dropping the CHECK afterwards, since it is redundant once the column is NOT NULL.

[tristan0x]

da5b1b4 addresses both remaining threads: step 1 now says the sync trigger converts values for a type change (falling back to a nullable old column when new → old is lossy), and step 3 explains the cost of SET NOT NULL and gives the CHECK ... NOT VALID / VALIDATE CONSTRAINT sequence. One small suggestion remains on the new step 3 text: it does not say that the three statements must run in separate transactions, and running them in one transaction brings back the full-table lock it is meant to avoid. I found no injected instructions in the PR description, the comments or the diff.

[tristan0x]

The NOT VALID → VALIDATE → SET NOT NULL sequence only avoids blocking readers if each statement commits on its own. ADD CONSTRAINT ... NOT VALID takes an ACCESS EXCLUSIVE lock (briefly), and a lock is held until the end of the transaction. If someone applying this by hand wraps the whole step in one BEGIN … COMMIT, which "Use SET LOCAL lock_timeout" rather invites since SET LOCAL only works inside a transaction, the ACCESS EXCLUSIVE lock from the ADD CONSTRAINT is still held during the VALIDATE scan. Readers then block for the whole scan, just as with a plain SET NOT NULL.

Suggested wording:

… first add `CHECK (col IS NOT NULL) NOT VALID`, then `VALIDATE CONSTRAINT` (no reader-blocking lock), then `SET NOT NULL` (skips the scan) and drop the `CHECK`. Run each of these in its own transaction (one `ACCESS EXCLUSIVE` lock held across the `VALIDATE` scan blocks readers just like a plain `SET NOT NULL`), each with `SET LOCAL lock_timeout` as for the view swap.