Repository navigation
fix: make migration 65 compatible with pg12+ - #1400
Conversation
Coverage Report for CI Build 35132410023Coverage decreased (-0.07%) to 82.76%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions18 previously-covered lines in 2 files lost coverage.
Coverage Stats💛 - Coveralls |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it touches the core migration runner (a path that executes against every tenant database) and rewrites an already-shipped migration file, a human look would still be worthwhile.
What was reviewed:
- SQL generation semantics in migration 0065 (pg15+
NULLS NOT DISTINCTvs. the pre-15COALESCE/IS NULLfallback) — the null-handling looks equivalent between branches. - migrate.ts's new generate-sql path — it validates exactly one non-empty SQL string before substituting it, so
CONCURRENTLYhandling and invalid-index repair still run against the resolved statement. - Rewriting migration 65 changes its hash for tenants that already applied it; confirmed
dbRefreshMigrationHashesOnMismatchdefaults to true, so this self-heals unless an operator explicitly setsDB_ALLOW_MIGRATION_REFRESH=false. - Confirmed the concurrent-index-guard's literal-skipping regex correctly ignores the
CREATE INDEX CONCURRENTLYtext embedded in the new dollar-quoted strings, which explains the updated expected query count in that test.
Extended reasoning...
Overview
The PR rewrites migration 0065's SQL as a "generate-sql" script that emits a version-dependent CREATE UNIQUE INDEX CONCURRENTLY statement (native NULLS NOT DISTINCT on pg15+, a COALESCE/IS NULL-based equivalent on older servers), and teaches the migration runner (migrate.ts) to detect this marker, execute the generator query first, validate it returns exactly one non-empty SQL string, and substitute that into the normal transform/repair/execute pipeline. Tests were updated in lockstep: concurrent-index-guard.test.ts adjusts an expected call count because the raw checked-in file no longer contains a literal CREATE INDEX CONCURRENTLY (it's now embedded inside dollar-quoted string literals that the guard's literal-skipper correctly treats as non-SQL), and migrate.test.ts adds a generated parameter across existing repair/keep/Oriole tests plus new cases for malformed generator output (empty rows, null/non-string/blank sql, multiple rows) and failure-to-record-on-error paths.
Security risks
No injection or auth-relevant surface here — the generated SQL is fully static (built from current_setting('server_version_num'), no external input), and the new validation in migrate.ts (row count, string type, non-blank) guards against a malformed generator producing no-op or garbage SQL that could otherwise silently mark a migration as applied without doing anything.
Level of scrutiny
This is infrastructure code (the shared migration runner used for every tenant database and every migration file), which argues for a higher bar than a typical bug fix, even though the diff itself is compact and well tested. I also independently checked the two candidate issues that the automated hunt considered and ruled out — changing migration 65's text changes its stored hash for databases that already ran it, and both version branches share an index name under IF NOT EXISTS — and found the first is mitigated by default configuration (hash mismatches auto-refresh unless an operator opts out) and the second requires a narrow retry-after-major-upgrade scenario that doesn't affect correctness of the resulting index, just its exact definition.
Other factors
Test coverage is thorough for the new code path (generated vs. non-generated variants of existing tests, plus new negative cases for malformed generator output and failure-to-persist-on-error). Given the change modifies a previously-merged, already-shipped migration file and adds a new general mechanism to the migration engine, I'm deferring rather than approving so a maintainer can weigh in on the design choice, even though no concrete bug was found.
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
Migration 65 uses
NULLS NOT DISTINCTwhich is pg15 syntax but we need to support pg12+.What is the new behavior?
Update migration in place to use new syntax according to version and for old, ise simpler empty string coalesce for uniqueness.
Additional context
Related to #1341