diff --git a/apps/server/src/database/concurrent-indexes.spec.ts b/apps/server/src/database/concurrent-indexes.spec.ts new file mode 100644 index 00000000..cc3b8e6c --- /dev/null +++ b/apps/server/src/database/concurrent-indexes.spec.ts @@ -0,0 +1,89 @@ +import * as path from 'path'; +import { readFileSync } from 'fs'; + +// Mock ONLY kysely's `sql.raw(...).execute()` so we can observe what +// ensureConcurrentIndexes runs and how it handles failures, without a DB. +const execMock = jest.fn((_db: unknown) => Promise.resolve(undefined)); +const rawMock = jest.fn((_stmt: string) => ({ execute: execMock })); +jest.mock('kysely', () => { + const actual = jest.requireActual('kysely'); + return { ...actual, sql: { ...actual.sql, raw: rawMock } }; +}); + +import { CONCURRENT_INDEXES, ensureConcurrentIndexes } from './concurrent-indexes'; + +describe('ensureConcurrentIndexes', () => { + const fakeDb = { __topLevelKysely: true } as never; + + beforeEach(() => { + execMock.mockReset().mockResolvedValue(undefined); + rawMock.mockClear(); + }); + + it('runs every registered index CONCURRENTLY, IF NOT EXISTS, 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; + expect(stmt).toContain('CREATE INDEX CONCURRENTLY'); + expect(stmt).toContain('IF NOT EXISTS'); + } + // Executed against the top-level db (a transaction would forbid CONCURRENTLY). + for (const call of execMock.mock.calls) { + expect(call[0]).toBe(fakeDb); + } + expect(onLog).toHaveBeenCalledTimes(CONCURRENT_INDEXES.length); + }); + + 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. + execMock + .mockRejectedValueOnce(new Error('relation "pages" does not exist')) + .mockResolvedValue(undefined); + const onLog = jest.fn(); + + await expect( + ensureConcurrentIndexes(fakeDb, onLog), + ).resolves.toBeUndefined(); + + expect(execMock).toHaveBeenCalledTimes(CONCURRENT_INDEXES.length); + // The failure surfaced with an error argument for the caller to log. + const errored = onLog.mock.calls.filter((c) => c[1] !== undefined); + expect(errored).toHaveLength(1); + expect(String(errored[0][1])).toContain('does not exist'); + }); +}); + +// DRIFT GUARD: each CONCURRENT_INDEXES entry pre-builds an index that a plain +// migration ALSO creates with `IF NOT EXISTS`. If the two expressions diverge, +// Postgres would treat them as different indexes and the pre-build would NOT +// make the migration a no-op. Assert the migration files still contain each +// index's functional expression. +describe('CONCURRENT_INDEXES parity with the migrations', () => { + const migrationsDir = path.join(__dirname, 'migrations'); + const files = [ + '20260705T120000-perf-indexes.ts', + '20260706T120000-search-lookup-trgm.ts', + ].map((f) => readFileSync(path.join(migrationsDir, f), 'utf8')); + const allMigrationSrc = files.join('\n'); + + it.each(CONCURRENT_INDEXES.map((i) => [i.name, i]))( + 'migration source still creates %s with the same expression', + (_name, idx) => { + // Extract the `USING gin ((...expr...) gin_trgm_ops)` tail from the + // canonical create and assert the migration source contains it verbatim. + const m = (idx as { create: string }).create.match( + /ON \w+ (USING gin .+)$/, + ); + expect(m).not.toBeNull(); + const expr = (m as RegExpMatchArray)[1]; + expect(allMigrationSrc).toContain(expr); + // And the migration must build it by the SAME index name. + expect(allMigrationSrc).toContain( + `CREATE INDEX IF NOT EXISTS ${(idx as { name: string }).name}`, + ); + }, + ); +}); diff --git a/apps/server/src/database/concurrent-indexes.ts b/apps/server/src/database/concurrent-indexes.ts new file mode 100644 index 00000000..a337d6c5 --- /dev/null +++ b/apps/server/src/database/concurrent-indexes.ts @@ -0,0 +1,84 @@ +import { Kysely, sql } from 'kysely'; + +/** + * Indexes that MUST be built with `CREATE INDEX CONCURRENTLY` so an auto-deploy + * migration never takes a `SHARE` lock that blocks writes on a hot table + * (`pages`) for the — potentially minutes-long — GIN trigram build (#495 item 12). + * + * Kysely runs each migration INSIDE a transaction (Postgres has transactional + * DDL), and `CREATE INDEX CONCURRENTLY` cannot run inside a transaction block, so + * these cannot live in an ordinary migration. Instead {@link ensureConcurrentIndexes} + * 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. + * + * The `create` text is the CANONICAL definition — it MUST match the migration's + * `IF NOT EXISTS` create expression exactly (same functional expression + opclass) + * or Postgres would treat them as two different indexes. + */ +export const CONCURRENT_INDEXES: ReadonlyArray<{ + name: string; + create: string; +}> = [ + { + // #348 perf-indexes — pages.title trigram (coalesce-free functional expr). + name: 'idx_pages_title_trgm', + create: + 'CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_pages_title_trgm ' + + 'ON pages USING gin ((LOWER(f_unaccent(title))) gin_trgm_ops)', + }, + { + // #348 perf-indexes — users.name trigram (member search-suggest). + name: 'idx_users_name_trgm', + create: + 'CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_users_name_trgm ' + + 'ON users USING gin ((LOWER(f_unaccent(name))) gin_trgm_ops)', + }, + { + // #443 search-lookup-trgm — pages.text_content trigram (the slow, large one). + name: 'idx_pages_text_content_trgm', + create: + 'CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_pages_text_content_trgm ' + + 'ON pages USING gin ((LOWER(f_unaccent(text_content))) gin_trgm_ops)', + }, +]; + +/** + * Best-effort, non-transactional pre-build of {@link CONCURRENT_INDEXES}. Run + * BEFORE the migrator so the blocking `CREATE INDEX` in the corresponding + * migration becomes an `IF NOT EXISTS` no-op. + * + * `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. + */ +export async function ensureConcurrentIndexes( + db: Kysely, + onLog?: (message: string, error?: unknown) => void, +): Promise { + 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). + onLog?.( + `Concurrent index pre-build skipped for ${idx.name} ` + + `(will fall back to the in-migration build)`, + error, + ); + } + } +} diff --git a/apps/server/src/database/migrations/20260705T120000-perf-indexes.ts b/apps/server/src/database/migrations/20260705T120000-perf-indexes.ts index b39112d4..d9d97243 100644 --- a/apps/server/src/database/migrations/20260705T120000-perf-indexes.ts +++ b/apps/server/src/database/migrations/20260705T120000-perf-indexes.ts @@ -33,16 +33,18 @@ import { type Kysely, sql } from 'kysely'; * - comments: `findPageComments` does WHERE page_id ORDER BY id ASC, but only * `(page_id)` exists → extra sort. * - * DEPLOY-TIME LOCK WARNING: these are plain (non-CONCURRENT) CREATE INDEX - * statements — CONCURRENTLY is impossible because Kysely runs each migration in a - * transaction. They take a SHARE lock that BLOCKS writes (INSERT/UPDATE/DELETE) on - * pages/users/groups/comments/page_history for the duration of the build. The two - * GIN trigram builds on pages.title / users.name are the slow ones and can take - * minutes on a large tenant → a write-outage window during the deploy migration. - * For large installations, run this migration in a maintenance window, or build - * the trigram indexes out-of-band with CREATE INDEX CONCURRENTLY before deploying - * (then this migration's `IF NOT EXISTS` is a no-op). Small/typical tenants are - * unaffected. + * DEPLOY-TIME LOCK: these are plain (non-CONCURRENT) CREATE INDEX statements — + * CONCURRENTLY is impossible HERE because Kysely runs each migration in a + * transaction. The two GIN trigram builds on pages.title / users.name are the + * slow ones and would take a SHARE lock that BLOCKS writes on pages/users for + * minutes on a large tenant. To avoid that, `ensureConcurrentIndexes` + * (database/concurrent-indexes.ts) now pre-builds BOTH trigram indexes with + * CREATE INDEX CONCURRENTLY (no transaction) BEFORE the migrator runs, so on an + * existing DB the two `IF NOT EXISTS` trigram creates below no-op and no write + * lock is taken. On a fresh DB the pre-build is skipped and they build on an + * empty table. Keep the two trigram creates in lockstep with their CANONICAL + * definitions in CONCURRENT_INDEXES. (The plain b-tree indexes further down are + * fast metadata-only builds; they are not pre-built.) */ export async function up(db: Kysely): Promise { // Index-compatible, output-identical redefinition of f_unaccent (see header). diff --git a/apps/server/src/database/migrations/20260706T120000-search-lookup-trgm.ts b/apps/server/src/database/migrations/20260706T120000-search-lookup-trgm.ts index c28f4cf0..299e1980 100644 --- a/apps/server/src/database/migrations/20260706T120000-search-lookup-trgm.ts +++ b/apps/server/src/database/migrations/20260706T120000-search-lookup-trgm.ts @@ -26,13 +26,16 @@ import { type Kysely, sql } from 'kysely'; * to update than b-trees); on the small instances this fork targets that cost * is acceptable and the read win on agent lookups is the priority. * - * DEPLOY-TIME LOCK WARNING: plain (non-CONCURRENT) CREATE INDEX — Kysely runs - * each migration in a transaction, so CONCURRENTLY is impossible. The build takes - * a SHARE lock that BLOCKS writes on `pages` for its duration. The text_content - * GIN build is the slow one and can take minutes on a large tenant. For large - * installations, run this in a maintenance window or build the index out-of-band - * with CREATE INDEX CONCURRENTLY before deploying (then `IF NOT EXISTS` no-ops - * here). Small/typical tenants are unaffected. + * DEPLOY-TIME LOCK: this is a plain (non-CONCURRENT) CREATE INDEX — Kysely runs + * each migration in a transaction, so CONCURRENTLY is impossible HERE, and the + * build would take a SHARE lock that BLOCKS writes on `pages` for its duration + * (the text_content GIN build can take minutes on a large tenant). To avoid that, + * `ensureConcurrentIndexes` (database/concurrent-indexes.ts) now pre-builds this + * index with CREATE INDEX CONCURRENTLY (no transaction) BEFORE the migrator runs, + * so on an existing DB the `IF NOT EXISTS` below no-ops and no write lock is taken. + * On a fresh DB the pre-build is skipped and this builds it on an empty table + * where the lock is irrelevant. Keep this create in lockstep with the CANONICAL + * definition in CONCURRENT_INDEXES (same expression + opclass). */ export async function up(db: Kysely): Promise { // The title predicate is served by #348's idx_pages_title_trgm — see header. diff --git a/apps/server/src/database/services/migration.service.ts b/apps/server/src/database/services/migration.service.ts index 2b63f728..e70d28c4 100644 --- a/apps/server/src/database/services/migration.service.ts +++ b/apps/server/src/database/services/migration.service.ts @@ -4,6 +4,7 @@ import { promises as fs } from 'fs'; import { Migrator, FileMigrationProvider } from 'kysely'; import { InjectKysely } from 'nestjs-kysely'; import { KyselyDB } from '@docmost/db/types/kysely.types'; +import { ensureConcurrentIndexes } from '@docmost/db/concurrent-indexes'; @Injectable() export class MigrationService { @@ -12,6 +13,16 @@ export class MigrationService { constructor(@InjectKysely() private readonly db: KyselyDB) {} async migrateToLatest(): Promise { + // Build write-blocking trigram indexes CONCURRENTLY (no transaction) BEFORE + // the migrator runs, so the corresponding in-migration `CREATE INDEX IF NOT + // EXISTS` no-ops instead of taking a SHARE lock on `pages` during deploy + // (#495). Best-effort: on a fresh DB (no `pages`/`f_unaccent` yet) this is a + // no-op and the migrations build the index normally. + await ensureConcurrentIndexes(this.db, (message, error) => { + if (error) this.logger.warn(`${message}: ${String(error)}`); + else this.logger.log(message); + }); + const migrator = new Migrator({ db: this.db, provider: new FileMigrationProvider({