diff --git a/apps/server/src/database/concurrent-indexes.spec.ts b/apps/server/src/database/concurrent-indexes.spec.ts index cc3b8e6c..cc3247bc 100644 --- a/apps/server/src/database/concurrent-indexes.spec.ts +++ b/apps/server/src/database/concurrent-indexes.spec.ts @@ -20,13 +20,20 @@ describe('ensureConcurrentIndexes', () => { rawMock.mockClear(); }); - it('runs every registered index CONCURRENTLY, IF NOT EXISTS, outside a transaction', async () => { + it('redefines the inlinable f_unaccent BEFORE building any index, then builds each CONCURRENTLY outside a transaction', async () => { const onLog = jest.fn(); await ensureConcurrentIndexes(fakeDb, onLog); - expect(rawMock).toHaveBeenCalledTimes(CONCURRENT_INDEXES.length); - for (const call of rawMock.mock.calls) { - const stmt = call[0] as string; + // One f_unaccent redefine + one statement per index. + expect(rawMock).toHaveBeenCalledTimes(CONCURRENT_INDEXES.length + 1); + const statements = rawMock.mock.calls.map((c) => c[0] as string); + // ORDER: the f_unaccent redefine MUST be first — otherwise an existing + // tenant's old 2-arg f_unaccent makes every CONCURRENTLY build fail. + expect(statements[0]).toContain('CREATE OR REPLACE FUNCTION f_unaccent'); + expect(statements[0]).toContain('SELECT public.unaccent($1)'); + expect(statements[0]).not.toContain('CREATE INDEX'); + // The rest are the CONCURRENTLY index builds. + for (const stmt of statements.slice(1)) { expect(stmt).toContain('CREATE INDEX CONCURRENTLY'); expect(stmt).toContain('IF NOT EXISTS'); } @@ -34,12 +41,31 @@ describe('ensureConcurrentIndexes', () => { for (const call of execMock.mock.calls) { expect(call[0]).toBe(fakeDb); } - expect(onLog).toHaveBeenCalledTimes(CONCURRENT_INDEXES.length); + expect(onLog).toHaveBeenCalledTimes(CONCURRENT_INDEXES.length + 1); + }); + + it('still builds the indexes when the f_unaccent redefine fails (fresh DB, best-effort)', async () => { + // Redefine (first execute) throws; the index loop must still run. + execMock + .mockRejectedValueOnce(new Error('extension "unaccent" does not exist')) + .mockResolvedValue(undefined); + const onLog = jest.fn(); + + await expect( + ensureConcurrentIndexes(fakeDb, onLog), + ).resolves.toBeUndefined(); + + // 1 redefine attempt + one per index. + expect(execMock).toHaveBeenCalledTimes(CONCURRENT_INDEXES.length + 1); + const errored = onLog.mock.calls.filter((c) => c[1] !== undefined); + expect(errored).toHaveLength(1); + expect(String(errored[0][1])).toContain('unaccent'); }); it('is best-effort: a failing index does not abort the rest and is reported', async () => { - // Fail the FIRST index; the remaining ones must still be attempted. + // Redefine ok; fail the FIRST index; the remaining ones must still be tried. execMock + .mockResolvedValueOnce(undefined) // f_unaccent redefine .mockRejectedValueOnce(new Error('relation "pages" does not exist')) .mockResolvedValue(undefined); const onLog = jest.fn(); @@ -48,8 +74,7 @@ describe('ensureConcurrentIndexes', () => { ensureConcurrentIndexes(fakeDb, onLog), ).resolves.toBeUndefined(); - expect(execMock).toHaveBeenCalledTimes(CONCURRENT_INDEXES.length); - // The failure surfaced with an error argument for the caller to log. + expect(execMock).toHaveBeenCalledTimes(CONCURRENT_INDEXES.length + 1); const errored = onLog.mock.calls.filter((c) => c[1] !== undefined); expect(errored).toHaveLength(1); expect(String(errored[0][1])).toContain('does not exist'); diff --git a/apps/server/src/database/concurrent-indexes.ts b/apps/server/src/database/concurrent-indexes.ts index a337d6c5..c17e0652 100644 --- a/apps/server/src/database/concurrent-indexes.ts +++ b/apps/server/src/database/concurrent-indexes.ts @@ -11,12 +11,34 @@ import { Kysely, sql } from 'kysely'; * builds them out-of-band (no transaction) BEFORE the migrator runs; the matching * migrations keep a plain `CREATE INDEX IF NOT EXISTS` as a backstop, which then * no-ops because the index already exists. So: - * - existing prod DB, incremental deploy: pre-build runs CONCURRENTLY (no write - * lock), migration's IF NOT EXISTS no-ops → the write-blocking build is gone; - * - fresh DB (or a DB that has not yet created `pages` / `f_unaccent`): the - * pre-build fails and is swallowed (best-effort), and the migration builds the - * index normally on an empty/small table where the lock is irrelevant. - * Worst case therefore equals the previous behaviour; best case removes the lock. + * - existing prod DB, incremental deploy: the pre-build FIRST redefines + * `f_unaccent` to its index-inlinable 1-arg form (see below), THEN builds each + * index CONCURRENTLY (no write lock); the migration's IF NOT EXISTS no-ops → + * the write-blocking build is gone; + * - fresh DB (or a DB whose `pages` / `unaccent` extension does not exist yet): + * the pre-build fails and is swallowed (best-effort), and the migration builds + * the index normally on an empty/small table where the lock is irrelevant. + * + * NOT a strict "worst case = previous behaviour": + * - The f_unaccent form matters. The trigram expressions use `LOWER(f_unaccent(col))`. + * The INDEX-INLINABLE 1-arg `f_unaccent(text)` (`SELECT public.unaccent($1)`) + * is created INSIDE migration 20260705, which runs AFTER this pre-build. On an + * existing tenant the live `f_unaccent` is still the OLD 2-arg form from + * 20250729, which Postgres CANNOT inline into a CONCURRENTLY index expression + * (`function unaccent(unknown, text) does not exist ... during inlining`) — the + * build fails outright. So this pre-build redefines `f_unaccent` to the 1-arg + * form FIRST (idempotent, output-identical, in lockstep with 20260705). Without + * that redefine the whole feature is dead-on-arrival for the very case it + * targets. + * - An INTERRUPTED `CREATE INDEX CONCURRENTLY` (killed pod, cancelled query) with + * a working `f_unaccent` leaves an INVALID index behind. A subsequent name-based + * `IF NOT EXISTS` — in BOTH this pre-build and the migration backstop — sees the + * name and skips, so the invalid index is NEVER repaired automatically and the + * query keeps seq-scanning until an operator `DROP INDEX`es it. The old + * in-transaction build could not leave an invalid index (a failed tx rolled the + * whole index back), so this is a genuinely new failure mode, not "= previous". + * (A future hardening could `DROP` an `indisvalid = false` index before + * rebuilding; not done here.) * * The `create` text is the CANONICAL definition — it MUST match the migration's * `IF NOT EXISTS` create expression exactly (same functional expression + opclass) @@ -49,6 +71,24 @@ export const CONCURRENT_INDEXES: ReadonlyArray<{ }, ]; +/** + * Idempotent, output-identical redefinition of `f_unaccent` to the INDEX-INLINABLE + * 1-arg form. MUST stay byte-for-byte in lockstep with migration + * `20260705T120000-perf-indexes.ts` (same signature + body): the trigram indexes + * above only build CONCURRENTLY once `f_unaccent(text)` inlines, and on an existing + * tenant it is still the old 2-arg form until that migration runs — which is AFTER + * this pre-build. + */ +const F_UNACCENT_REDEF = ` + CREATE OR REPLACE FUNCTION f_unaccent(text) + RETURNS text + LANGUAGE sql + IMMUTABLE PARALLEL SAFE STRICT + AS $func$ + SELECT public.unaccent($1); + $func$ +`; + /** * Best-effort, non-transactional pre-build of {@link CONCURRENT_INDEXES}. Run * BEFORE the migrator so the blocking `CREATE INDEX` in the corresponding @@ -57,23 +97,41 @@ export const CONCURRENT_INDEXES: ReadonlyArray<{ * `db` MUST be the top-level Kysely instance (NOT a transaction): each statement * then executes on its own connection with no surrounding `BEGIN`, which is * required for `CONCURRENTLY`. Every statement is independent and swallowed on - * error (a missing `pages`/`f_unaccent` on a fresh DB, an unsupported driver - * path, a permissions gap): the migration backstop still builds the index, so a - * failure here is never fatal. `onLog` reports progress/failures for the caller - * to route to its logger. + * error: the migration backstop still builds the index, so a failure here is + * never fatal. `onLog` reports progress/failures for the caller to route to its + * logger. + * + * ORDER MATTERS: redefine `f_unaccent` to the inlinable form BEFORE the index + * loop — otherwise an existing tenant's old 2-arg `f_unaccent` makes every + * CONCURRENTLY build fail and the feature is a no-op (see the module docstring). */ export async function ensureConcurrentIndexes( db: Kysely, onLog?: (message: string, error?: unknown) => void, ): Promise { + // Make f_unaccent inlinable FIRST. On a fresh DB (no `unaccent` extension yet) + // this throws harmlessly and is swallowed — the migration builds everything. + try { + await sql.raw(F_UNACCENT_REDEF).execute(db); + onLog?.('f_unaccent redefined to the index-inlinable 1-arg form'); + } catch (error) { + onLog?.( + 'f_unaccent redefine skipped (fresh DB / no unaccent extension) — ' + + 'the migration will define it and build the indexes', + error, + ); + } + for (const idx of CONCURRENT_INDEXES) { try { await sql.raw(idx.create).execute(db); onLog?.(`Concurrent index ensured: ${idx.name}`); } catch (error) { // Non-fatal by design — the migration's IF NOT EXISTS create is the - // backstop. Common benign cause: the table/function does not exist yet on - // a fresh DB (the migrations will build the index instead). + // backstop. Benign on a fresh DB (the table/extension does not exist yet). + // NOT benign, but still swallowed, on an existing tenant whose f_unaccent + // could not be redefined above (then the migration builds the index NON- + // concurrently under the SHARE lock this feature meant to avoid). onLog?.( `Concurrent index pre-build skipped for ${idx.name} ` + `(will fall back to the in-migration build)`,