|
| 1 | +# Pre-flight Migration Review (DDL Safety) |
| 2 | + |
| 3 | +**Status:** approved design, pending implementation |
| 4 | +**Date:** 2026-09-04 |
| 5 | +**Branch:** `feat/preflight-migration-review` |
| 6 | + |
| 7 | +## Problem |
| 8 | + |
| 9 | +`SchemaChangeTrackingService` is retrospective: it snapshots the schema and reports |
| 10 | +what already changed. Nothing answers the question a DBA actually asks *before* |
| 11 | +running a migration — "will this lock the table, and for how long?" |
| 12 | + |
| 13 | +Agent chat can approximate an answer today (`get_schema`, `get_table_growth`, |
| 14 | +`execute_sql`), but it answers from model recall. DDL lock semantics are precise, |
| 15 | +version-dependent and counterintuitive, and a wrong answer arrives with the same |
| 16 | +confidence as a right one. For a question whose entire purpose is deciding whether |
| 17 | +to run something against production, that is worse than no tool. |
| 18 | + |
| 19 | +Evidence this is a real hazard, not a theoretical one: while writing this spec the |
| 20 | +author twice asserted that a volatile `DEFAULT now()` forces a table rewrite. It |
| 21 | +does not — `now()` is STABLE. Measured against a real Postgres, not recalled. |
| 22 | + |
| 23 | +## Scope |
| 24 | + |
| 25 | +A deterministic analyzer exposed as **one MCP tool**, `analyze_migration`. No new |
| 26 | +UI section, no new frontend surface. The Agent remains the interface; the tool |
| 27 | +makes its verdict verifiable. |
| 28 | + |
| 29 | +Explicitly out of scope: executing migrations, rewriting them automatically, a |
| 30 | +history of past analyses, MySQL support. |
| 31 | + |
| 32 | +## Convention this follows |
| 33 | + |
| 34 | +The codebase already draws a consistent line, and this sits on the deterministic |
| 35 | +side of it (22 of 165 services touch an LLM; 40 of 42 brain services do not): |
| 36 | + |
| 37 | +- **The database or arithmetic over its catalog decides the verdict.** |
| 38 | + `IndexAdvisorService` scores index health with explicit arithmetic and estimates |
| 39 | + impact via `hypopg` + `pg_class`. `SchemaEvolutionRiskService` tiers risk from |
| 40 | + size thresholds with no model at all. |
| 41 | +- **The LLM narrates.** `ExplainPlanService` lets Postgres produce the plan — ground |
| 42 | + truth — and uses a model only to interpret it. |
| 43 | + |
| 44 | +`analyze_migration` returns structured facts. The Agent writes the prose. |
| 45 | + |
| 46 | +## Design |
| 47 | + |
| 48 | +### Pipeline |
| 49 | + |
| 50 | +1. **Parse** with JSqlParser (already a dependency) into a fact set: |
| 51 | + operation, table, column, target type, default expression, nullability, |
| 52 | + `CONCURRENTLY` presence. |
| 53 | +2. **Fail closed.** Unparseable or unrecognised input returns `UNKNOWN` with |
| 54 | + `safeToRun: false`. Never "looks fine." (CLAUDE.md: leading-keyword |
| 55 | + classification is how a non-admin wiped tables through the Editor.) |
| 56 | +3. **Classify** against a per-dialect rule table keyed on |
| 57 | + `(operation, discriminating conditions)`, yielding lock mode + rewrite flag. |
| 58 | +4. **Scale** by live catalog data — `pg_total_relation_size()`, row estimate — to |
| 59 | + convert "rewrites the table" into a duration bucket. |
| 60 | + |
| 61 | +### The discriminator that matters |
| 62 | + |
| 63 | +Identical syntax, different consequences, decided by `pg_proc.provolatile` of the |
| 64 | +default expression's function — queried live, so any function classifies correctly |
| 65 | +rather than only hardcoded ones: |
| 66 | + |
| 67 | +| Statement | provolatile | Rewrites | |
| 68 | +|---|---|---| |
| 69 | +| `ADD COLUMN c text` | — | no | |
| 70 | +| `ADD COLUMN c text DEFAULT 'x'` | — (constant) | no | |
| 71 | +| `ADD COLUMN c timestamptz DEFAULT now()` | `s` stable | **no** | |
| 72 | +| `ADD COLUMN c uuid DEFAULT gen_random_uuid()` | `v` volatile | **yes** | |
| 73 | +| `ADD COLUMN c double precision DEFAULT random()` | `v` volatile | **yes** | |
| 74 | +| `ADD COLUMN c timestamptz DEFAULT clock_timestamp()` | `v` volatile | **yes** | |
| 75 | + |
| 76 | +### Measured rule table (Postgres 18.4) |
| 77 | + |
| 78 | +Every row verified against a live instance via `pg_locks` and |
| 79 | +`pg_relation_filenode()`, not from documentation or recall: |
| 80 | + |
| 81 | +| Operation | Lock | Rewrites | Verdict | |
| 82 | +|---|---|---|---| |
| 83 | +| `ADD COLUMN` (no default / constant / stable default) | AccessExclusive | no | SAFE | |
| 84 | +| `ADD COLUMN` (volatile default) | AccessExclusive | **yes** | DANGER if large | |
| 85 | +| `ADD COLUMN NOT NULL` (no default, non-empty table) | — | — | FAILS: `contains null values` | |
| 86 | +| `ALTER COLUMN TYPE` narrowing (`text`→`varchar(50)`) | AccessExclusive + Share | **yes** | DANGER if large | |
| 87 | +| `ALTER COLUMN TYPE` widening (`varchar(50)`→`varchar(100)`) | AccessExclusive | no | SAFE | |
| 88 | +| `SET NOT NULL` on existing column | AccessExclusive | no | CAUTION (full scan to validate) | |
| 89 | +| `DROP COLUMN` | AccessExclusive | no | SAFE (metadata only) | |
| 90 | +| `RENAME COLUMN` | AccessExclusive | no | SAFE (breaks app code, not the DB) | |
| 91 | +| `CREATE INDEX` | AccessExclusive + Share | n/a | DANGER: blocks writes for the build | |
| 92 | +| `CREATE INDEX CONCURRENTLY` | ShareUpdateExclusive | n/a | SAFE, can leave an invalid index | |
| 93 | + |
| 94 | +**Lock type and duration are independent axes.** `ADD COLUMN` with no default takes |
| 95 | +AccessExclusive — the strongest lock — yet is harmless because it is held for |
| 96 | +milliseconds. The same lock across a 48M-row rewrite is the outage. The tool must |
| 97 | +report both; reporting the lock name alone is alarming and useless. |
| 98 | + |
| 99 | +### Output shape |
| 100 | + |
| 101 | +```json |
| 102 | +{ |
| 103 | + "dialect": "postgres", |
| 104 | + "verdict": "DANGER", |
| 105 | + "safeToRun": false, |
| 106 | + "operation": "ADD_COLUMN", |
| 107 | + "table": "orders", |
| 108 | + "lockMode": "AccessExclusiveLock", |
| 109 | + "blocks": ["read", "write"], |
| 110 | + "rewritesTable": true, |
| 111 | + "tableRows": 48000000, |
| 112 | + "tableSizeBytes": 9200000000, |
| 113 | + "estimatedDuration": "minutes-to-tens-of-minutes", |
| 114 | + "reason": "Default expression gen_random_uuid() is VOLATILE, forcing a full table rewrite.", |
| 115 | + "saferAlternative": "Add the column without a default, backfill in batches, then set the default.", |
| 116 | + "docsUrl": "https://www.postgresql.org/docs/18/sql-altertable.html", |
| 117 | + "confidence": "verified" |
| 118 | +} |
| 119 | +``` |
| 120 | + |
| 121 | +`estimatedDuration` is a coarse bucket, never a false-precision number — real time |
| 122 | +depends on disk, cache and load. |
| 123 | + |
| 124 | +### Placement |
| 125 | + |
| 126 | +- `MigrationRiskProvider` in `provider/api/`, implemented by |
| 127 | + `PostgresMigrationRiskProvider`, reached via `DatabaseDialect` — following the |
| 128 | + registry rule (no if/else on database type). |
| 129 | +- `MigrationRiskService` orchestrates parse → classify → scale. |
| 130 | +- MySQL: `UnsupportedDatabaseException`. Explicitly unsupported beats a confident |
| 131 | + wrong answer, and matches `IndexAdvisorService`'s Postgres-only catalog queries. |
| 132 | + |
| 133 | +### Authorization |
| 134 | + |
| 135 | +Read-only: catalog queries plus parsing, no DDL executed. Takes a caller-supplied |
| 136 | +`connectionId`, so it **must** call `accessControlService.assertCanReadConnectionContent` |
| 137 | +— per CLAUDE.md, no filter does this for you. |
| 138 | + |
| 139 | +## Verification |
| 140 | + |
| 141 | +Three layers, and layer 2 is the one that matters. |
| 142 | + |
| 143 | +1. **Unit tests** over the rule table. Proves consistency, *not* correctness — if a |
| 144 | + rule is encoded wrong, the test enshrines the error. |
| 145 | +2. **Execution against a real Postgres.** For each rule: run the DDL in a |
| 146 | + transaction, read the lock from `pg_locks`, compare `pg_relation_filenode()` |
| 147 | + before and after, roll back. The engine testifies about itself. This is what |
| 148 | + caught the `now()` error. Runs in CI so an upstream behaviour change fails the |
| 149 | + build instead of silently making the tool wrong. |
| 150 | +3. **Docs citation** per rule, so any verdict can be audited in ~30 seconds. |
| 151 | + |
| 152 | +Implementation note: rank locks by an explicit strength ordering. `max(mode)` is |
| 153 | +alphabetical — "ShareLock" sorts above "AccessExclusiveLock" — which misreported |
| 154 | +the strongest lock during the spike. |
| 155 | + |
| 156 | +## Known limits |
| 157 | + |
| 158 | +- Coverage is only the encoded shapes; everything else returns `UNKNOWN`. |
| 159 | +- Duration is a bucket, not a prediction. |
| 160 | +- Postgres only. |
| 161 | +- Verified against 18.4; older majors differ (pre-11 rewrites on *any* default). |
| 162 | + The rule table is version-tagged and the CI probe pins the same major. |
| 163 | + |
| 164 | +## MCP release checklist |
| 165 | + |
| 166 | +Per CLAUDE.md, a new MCP tool requires all of these in the same commit: |
| 167 | +tool definition + handler + result builder + summarizer, CLI dispatcher, CLI help |
| 168 | +text, `SKILL_BODY.md` tool table and count, `mcp/CLAUDE.md`, `mcp/README.md`, |
| 169 | +`mcp/package.json` minor bump. |
0 commit comments