From f919ced8c9b9109c0c0f715519f9aa30d41adc67 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 12 Jul 2026 00:37:07 +0300 Subject: [PATCH] =?UTF-8?q?fix(db):=20concurrent-pre-build=20=D0=B8=D0=BD?= =?UTF-8?q?=D0=B4=D0=B5=D0=BA=D1=81=D0=BE=D0=B2=20=E2=80=94=20=D0=BF=D0=B5?= =?UTF-8?q?=D1=80=D0=B5=D0=BE=D0=BF=D1=80=D0=B5=D0=B4=D0=B5=D0=BB=D1=8F?= =?UTF-8?q?=D1=82=D1=8C=20f=5Funaccent=20=D0=9F=D0=95=D0=A0=D0=95=D0=94=20?= =?UTF-8?q?=D1=86=D0=B8=D0=BA=D0=BB=D0=BE=D0=BC?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ревью #516: фича была мёртвой-при-рождении ровно на целевом кейсе. Все три CONCURRENT_INDEXES используют LOWER(f_unaccent(col)), но index-инлайнящаяся 1-арг форма f_unaccent (SELECT public.unaccent($1)) создаётся ВНУТРИ миграции 20260705, а ensureConcurrentIndexes зовётся ПЕРЕД мигратором. На существующем тенанте живой f_unaccent — ещё старая 2-арг форма из 20250729, которая НЕ инлайнится: CREATE INDEX CONCURRENTLY падает («function unaccent(unknown, text) does not exist … during inlining»), best-effort глотает, и мигратор строит индексы НЕ-concurrently под тем самым SHARE-локом (эмпирически подтверждено ревьюером на живом pg). Фикс: перед циклом идемпотентно переопределяем f_unaccent в 1-арг форму (output-identical, в lockstep с 20260705), в том же best-effort try/swallow. Плюс честный докстринг: убрал ложное «worst case = previous behaviour». Прерванный CONCURRENTLY оставляет INVALID-индекс, который name-based IF NOT EXISTS не чинит — новый режим отказа (старый in-tx build такого не оставлял). Тест: переопределялка вызывается ПЕРВОЙ (order), затем индексы CONCURRENTLY вне транзакции; провал переопределялки не рвёт цикл. Mutation-verify: убрать pre-loop redef → order-тест краснеет. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/database/concurrent-indexes.spec.ts | 41 ++++++++-- .../server/src/database/concurrent-indexes.ts | 82 ++++++++++++++++--- 2 files changed, 103 insertions(+), 20 deletions(-) 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)`,