diff --git a/internal/store/attachments.go b/internal/store/attachments.go index 26bb18a89..6fd386504 100644 --- a/internal/store/attachments.go +++ b/internal/store/attachments.go @@ -1489,9 +1489,11 @@ func (s *Store) WorkspaceItemSlugMap(workspaceID string) (map[string]string, err // which used to rewrite a mapped id sitting as the PREFIX of a // longer one (Codex round 26). See its doc. // -// FTS reindex via the existing rebuild helper happens AFTER the -// transaction commits — direct UPDATE bypasses the SQLite FTS -// triggers the same way ImportWorkspace's INSERTs do. +// Search needs no separate pass: on SQLite each UPDATE fires the +// items_fts_update / comments_fts_update triggers inside the +// transaction, and on Postgres search_vector is trigger-maintained. A +// post-commit rebuild that used to follow re-inserted every item and +// double-indexed it (BUG-2758). func (s *Store) RemapAttachmentReferencesInWorkspace(workspaceID string, oldToNew map[string]string) error { if len(oldToNew) == 0 { return nil @@ -1677,20 +1679,19 @@ func (s *Store) RemapAttachmentReferencesInWorkspace(workspaceID string, oldToNe } } + // The UPDATEs above fired items_fts_update, which already reindexed the + // rewritten content; there is no separate FTS pass (BUG-2758). if err := tx.Commit(); err != nil { return fmt.Errorf("commit remap: %w", err) } - - // Refresh FTS so search queries see the rewritten content. - s.rebuildFTSForWorkspace(workspaceID) return nil } // remapAttachmentRefs replaces "pad-attachment:OLD" with // "pad-attachment:NEW" for every (old, new) pair in the map. Pure // string operation — kept private so callers go through -// RemapAttachmentReferencesInWorkspace which also handles the FTS -// reindex. +// RemapAttachmentReferencesInWorkspace, whose UPDATEs fire the FTS +// triggers. // // IT TOKENIZES WITH attachmentRefRE, the SAME pattern the copy // planner enumerates references with, and rewrites only when the diff --git a/internal/store/export.go b/internal/store/export.go index c8c1cf8f5..4a75f8390 100644 --- a/internal/store/export.go +++ b/internal/store/export.go @@ -1223,38 +1223,19 @@ func (s *Store) ImportWorkspace(data *models.WorkspaceExport, newName string, ow } } + // No FTS pass follows the commit. On SQLite items_fts is an + // external-content table kept by the items_fts_* triggers, which fired for + // every row inserted above, inside this transaction. A second pass that + // re-inserted each row double-indexed it (FTS5 integrity-check reports the + // table malformed) and cost one write transaction per item after the + // commit, which stalled every other writer on the database (BUG-2758). if err := tx.Commit(); err != nil { return nil, fmt.Errorf("commit import: %w", err) } - // Rebuild FTS indexes for the new workspace (outside transaction) - s.rebuildFTSForWorkspace(ws.ID) - return ws, nil } -// rebuildFTSForWorkspace rebuilds the FTS index for all items in a workspace. -// This is needed after import because direct INSERTs bypass the FTS triggers. -// Only applicable to SQLite (PostgreSQL uses trigger-maintained tsvector columns). -func (s *Store) rebuildFTSForWorkspace(wsID string) { - if s.dialect.Driver() != DriverSQLite { - return - } - rows, err := s.db.Query(s.q(`SELECT rowid, title, content, tags FROM items WHERE workspace_id = ? AND deleted_at IS NULL`), wsID) - if err != nil { - return - } - defer rows.Close() - for rows.Next() { - var rowid int64 - var title, content, tags string - if err := rows.Scan(&rowid, &title, &content, &tags); err != nil { - continue - } - s.db.Exec(s.q(`INSERT INTO items_fts(rowid, title, content, tags) VALUES (?, ?, ?, ?)`), rowid, title, content, tags) - } -} - // remapFieldIDs replaces old UUIDs in a JSON fields string with their new IDs. // This handles relation fields (e.g. parent: "uuid") without needing to parse the schema. // diff --git a/internal/store/import_fts_test.go b/internal/store/import_fts_test.go new file mode 100644 index 000000000..6fdc25dbc --- /dev/null +++ b/internal/store/import_fts_test.go @@ -0,0 +1,151 @@ +package store + +import ( + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// BUG-2758. ImportWorkspace and RemapAttachmentReferencesInWorkspace each ran +// a post-commit rebuildFTSForWorkspace that re-inserted every item of the +// workspace into items_fts, one autocommit write per item. On SQLite items_fts +// is an EXTERNAL-CONTENT table (content='items') kept by the items_fts_* +// triggers, which had already indexed those rows inside the transaction. So +// the rebuild did two harmful things: +// +// - it double-indexed every imported item, which FTS5's own integrity-check +// reports as a malformed table. (Measured on the defect: search RESULTS +// looked right — one hit per item, and an edited-away word stopped +// matching — so the integrity check is the only instrument here that +// sees the corruption. Nothing below claims a search-result symptom.); +// - it turned an N-item import into N extra write transactions after the +// commit, a burst that stalled every other writer on the database. Under +// e2e load the stall outlasted the 30 s busy_timeout. +// +// The integrity check is the instrument for the defect. The search legs guard +// the other direction: with the rebuild gone, the TRIGGERS are the only thing +// indexing an imported or remapped row, and a search that finds it (and stops +// finding an edited-away word) is what shows they did. Postgres has no +// items_fts, so these legs are SQLite-only and skip there. + +func ftsIntegrity(t *testing.T, s *Store) error { + t.Helper() + _, err := s.db.Exec(`INSERT INTO items_fts(items_fts, rank) VALUES('integrity-check', 1)`) + return err +} + +func ftsImportBundle(content string) *models.WorkspaceExport { + const ts = "2026-09-24T12:00:00Z" + return &models.WorkspaceExport{ + Version: 1, + Workspace: models.WorkspaceExportMeta{Name: "FTS import", Slug: "fts-import", Settings: "{}"}, + Collections: []models.CollectionExport{ + {ID: "docs", Name: "Docs", Slug: "docs", Prefix: "DOC", Schema: `{"fields":[]}`, Settings: "{}", CreatedAt: ts, UpdatedAt: ts}, + }, + Items: []models.ItemExport{ + {ID: "old-1", CollectionID: "docs", Title: "First doc", Slug: "first-doc", Content: content, Fields: "{}", Tags: "[]", ItemNumber: 1, CreatedAt: ts, UpdatedAt: ts}, + {ID: "old-2", CollectionID: "docs", Title: "Second doc", Slug: "second-doc", Content: "unrelated body", Fields: "{}", Tags: "[]", ItemNumber: 2, CreatedAt: ts, UpdatedAt: ts}, + }, + } +} + +func skipUnlessSQLite(t *testing.T, s *Store) { + t.Helper() + if s.dialect.Driver() != DriverSQLite { + t.Skip("items_fts is SQLite-only; Postgres search_vector is a column on items") + } +} + +func TestImportWorkspaceLeavesSearchIndexConsistent(t *testing.T) { + t.Parallel() + s := testStore(t) + skipUnlessSQLite(t, s) + + // CONTROL: the check passes on a store nobody has imported into, so a + // failure below is about the import rather than about the fixture DB. + if err := ftsIntegrity(t, s); err != nil { + t.Fatalf("control: items_fts integrity-check fails before any import: %v", err) + } + + ws, err := s.ImportWorkspace(ftsImportBundle("the quokkaword lives here"), "", "", "") + if err != nil { + t.Fatal(err) + } + if err := ftsIntegrity(t, s); err != nil { + t.Fatalf("items_fts integrity-check after ImportWorkspace: %v (the import double-indexed its rows)", err) + } + + hits, err := s.SearchItems(ws.ID, "quokkaword") + if err != nil { + t.Fatal(err) + } + if len(hits) != 1 { + t.Fatalf("search for an imported word: %d hits, want 1", len(hits)) + } + + // Edit the imported item so the word is gone: the update trigger must + // reindex a row the insert trigger indexed during the import. + item, err := s.GetItemBySlug(ws.ID, "first-doc") + if err != nil || item == nil { + t.Fatalf("imported item: %v, %v", item, err) + } + replaced := "the body was rewritten" + if _, err := s.UpdateItem(item.ID, models.ItemUpdate{Content: &replaced}); err != nil { + t.Fatal(err) + } + stale, err := s.SearchItems(ws.ID, "quokkaword") + if err != nil { + t.Fatal(err) + } + if len(stale) != 0 { + t.Fatalf("search for a word the edit removed: %d hits, want 0", len(stale)) + } + fresh, err := s.SearchItems(ws.ID, "rewritten") + if err != nil { + t.Fatal(err) + } + if len(fresh) != 1 { + t.Fatalf("search for the edited word: %d hits, want 1", len(fresh)) + } +} + +// The remap is the second caller the rebuild had, and a bundle with +// attachments always reaches it. The import is asserted clean FIRST, so this +// leg fails on the remap alone rather than inheriting the import's damage. +func TestRemapAttachmentReferencesLeavesSearchIndexConsistent(t *testing.T) { + t.Parallel() + s := testStore(t) + skipUnlessSQLite(t, s) + + const oldID = "11111111-1111-4111-8111-111111111111" + const newID = "22222222-2222-4222-8222-222222222222" + ws, err := s.ImportWorkspace(ftsImportBundle("see ![shot](pad-attachment:"+oldID+") wombatword"), "", "", "") + if err != nil { + t.Fatal(err) + } + if err := ftsIntegrity(t, s); err != nil { + t.Fatalf("precondition: items_fts integrity-check after the import: %v", err) + } + + if err := s.RemapAttachmentReferencesInWorkspace(ws.ID, map[string]string{oldID: newID}); err != nil { + t.Fatal(err) + } + item, err := s.GetItemBySlug(ws.ID, "first-doc") + if err != nil || item == nil { + t.Fatalf("imported item: %v, %v", item, err) + } + if want := "pad-attachment:" + newID; !strings.Contains(item.Content, want) { + t.Fatalf("remap did not rewrite the reference; content %q", item.Content) + } + if err := ftsIntegrity(t, s); err != nil { + t.Fatalf("items_fts integrity-check after RemapAttachmentReferencesInWorkspace: %v", err) + } + hits, err := s.SearchItems(ws.ID, "wombatword") + if err != nil { + t.Fatal(err) + } + if len(hits) != 1 { + t.Fatalf("search after the remap: %d hits, want 1", len(hits)) + } +} diff --git a/internal/store/migration_095_items_fts_test.go b/internal/store/migration_095_items_fts_test.go new file mode 100644 index 000000000..7d131211d --- /dev/null +++ b/internal/store/migration_095_items_fts_test.go @@ -0,0 +1,252 @@ +package store + +import ( + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +const migration095 = "095_rebuild_items_fts.sql" + +// BUG-2758. Migration 095 repairs the duplicate items_fts postings a workspace +// import used to leave behind. The fixture reproduces that state the way the +// removed pass produced it, a second INSERT of an already-indexed row, and the +// control shows the fixture is really malformed before the repair runs. +func TestMigration095RepairsDoubleIndexedItems(t *testing.T) { + t.Parallel() + s := testStore(t) + skipUnlessSQLite(t, s) + + ws, err := s.ImportWorkspace(ftsImportBundle("the numbatword lives here"), "", "", "") + if err != nil { + t.Fatal(err) + } + if _, err := s.db.Exec(` + INSERT INTO items_fts(rowid, title, content, tags) + SELECT rowid, title, content, tags FROM items WHERE workspace_id = ?`, ws.ID); err != nil { + t.Fatal(err) + } + if err := ftsIntegrity(t, s); err == nil { + t.Fatal("control: the double-indexed fixture passes integrity-check, so it cannot show the repair works") + } + + reapply095(t, s) + if err := ftsIntegrity(t, s); err != nil { + t.Fatalf("items_fts integrity-check after %s: %v", migration095, err) + } + hits, err := s.SearchItems(ws.ID, "numbatword") + if err != nil { + t.Fatal(err) + } + if len(hits) != 1 { + t.Fatalf("search after the repair: %d hits, want 1", len(hits)) + } +} + +// reapply095 runs migration 095 again through the real runner +// (applySQLiteMigration: its own transaction, and the schema_migrations row), +// after forgetting that the store's construction already applied it. +func reapply095(t *testing.T, s *Store) { + t.Helper() + sqlText, err := migrationsFS.ReadFile("migrations/" + migration095) + if err != nil { + t.Fatal(err) + } + if _, err := s.db.Exec(`DELETE FROM schema_migrations WHERE version = ?`, migration095); err != nil { + t.Fatal(err) + } + if err := applySQLiteMigration(s.db, migration095, string(sqlText)); err != nil { + t.Fatalf("apply %s: %v", migration095, err) + } +} + +// A database that lost one of the three triggers (a state +// validateFTSInvariants only warns about) is maintained again after 095: a +// rebuild alone would repair the index once and then let it drift. +func TestMigration095RestoresAMissingTrigger(t *testing.T) { + t.Parallel() + s := testStore(t) + skipUnlessSQLite(t, s) + + ws, err := s.ImportWorkspace(ftsImportBundle("the bilbyword lives here"), "", "", "") + if err != nil { + t.Fatal(err) + } + if _, err := s.db.Exec(`DROP TRIGGER items_fts_update`); err != nil { + t.Fatal(err) + } + reapply095(t, s) + + var n int + if err := s.db.QueryRow(`SELECT count(*) FROM sqlite_master WHERE type = 'trigger' AND name = 'items_fts_update'`).Scan(&n); err != nil { + t.Fatal(err) + } + if n != 1 { + t.Fatalf("items_fts_update after %s: %d, want 1", migration095, n) + } + item, err := s.GetItemBySlug(ws.ID, "first-doc") + if err != nil || item == nil { + t.Fatalf("imported item: %v, %v", item, err) + } + replaced := "the body now says platypusword" + if _, err := s.UpdateItem(item.ID, models.ItemUpdate{Content: &replaced}); err != nil { + t.Fatal(err) + } + if err := ftsIntegrity(t, s); err != nil { + t.Fatalf("items_fts integrity-check after an edit: %v", err) + } + for word, want := range map[string]int{"bilbyword": 0, "platypusword": 1} { + hits, err := s.SearchItems(ws.ID, word) + if err != nil { + t.Fatal(err) + } + if len(hits) != want { + t.Fatalf("search %q after the edit: %d hits, want %d", word, len(hits), want) + } + } +} + +// A damaged database with no items_fts at all used to be a startup failure +// for a bare 'rebuild' (the migration errors and is not recorded). 095 +// creates the table and indexes what is there. +func TestMigration095RecreatesAMissingTable(t *testing.T) { + t.Parallel() + s := testStore(t) + skipUnlessSQLite(t, s) + + ws, err := s.ImportWorkspace(ftsImportBundle("the dingoword lives here"), "", "", "") + if err != nil { + t.Fatal(err) + } + for _, stmt := range []string{ + `DROP TRIGGER items_fts_insert`, + `DROP TRIGGER items_fts_update`, + `DROP TRIGGER items_fts_delete`, + `DROP TABLE items_fts`, + } { + if _, err := s.db.Exec(stmt); err != nil { + t.Fatalf("%s: %v", stmt, err) + } + } + reapply095(t, s) + + if err := ftsIntegrity(t, s); err != nil { + t.Fatalf("items_fts integrity-check after %s: %v", migration095, err) + } + hits, err := s.SearchItems(ws.ID, "dingoword") + if err != nil { + t.Fatal(err) + } + if len(hits) != 1 { + t.Fatalf("search after %s: %d hits, want 1", migration095, len(hits)) + } +} + +// An items_fts with ANOTHER definition is replaced rather than kept. A +// contentless table is the sharp case: FTS5 refuses 'rebuild' on one, so an +// IF NOT EXISTS create followed by a rebuild failed the migration (codex +// round 2). +func TestMigration095ReplacesAForeignDefinition(t *testing.T) { + t.Parallel() + s := testStore(t) + skipUnlessSQLite(t, s) + + ws, err := s.ImportWorkspace(ftsImportBundle("the possumword lives here"), "", "", "") + if err != nil { + t.Fatal(err) + } + for _, stmt := range []string{ + `DROP TRIGGER items_fts_insert`, + `DROP TRIGGER items_fts_update`, + `DROP TRIGGER items_fts_delete`, + `DROP TABLE items_fts`, + `CREATE VIRTUAL TABLE items_fts USING fts5(title, content, tags, content='')`, + } { + if _, err := s.db.Exec(stmt); err != nil { + t.Fatalf("%s: %v", stmt, err) + } + } + reapply095(t, s) + + var sqlText string + if err := s.db.QueryRow(`SELECT sql FROM sqlite_master WHERE name = 'items_fts'`).Scan(&sqlText); err != nil { + t.Fatal(err) + } + if !strings.Contains(sqlText, "content='items'") { + t.Fatalf("items_fts after %s is not the external-content table: %s", migration095, sqlText) + } + if err := ftsIntegrity(t, s); err != nil { + t.Fatalf("items_fts integrity-check after %s: %v", migration095, err) + } + hits, err := s.SearchItems(ws.ID, "possumword") + if err != nil { + t.Fatal(err) + } + if len(hits) != 1 { + t.Fatalf("search after %s: %d hits, want 1", migration095, len(hits)) + } +} + +// The repair is SQLite-only by construction: it lives in the SQLite chain, and +// Postgres migrates from pgmigrations/, which does not carry it. The second +// half is behavioural and runs only against Postgres (make test-pg): a fully +// migrated Postgres store has no items_fts relation for the statement to name. +// (A text scan of pgmigrations for "items_fts" is not the instrument: the GIN +// index on search_vector is named idx_items_fts.) +func TestMigration095IsNotInThePostgresChain(t *testing.T) { + t.Parallel() + sqliteNames, err := readMigrationNames(migrationsFS, "migrations") + if err != nil { + t.Fatal(err) + } + found := false + for _, n := range sqliteNames { + if n == migration095 { + found = true + } + } + if !found { + t.Fatalf("%s is not in the SQLite migration chain", migration095) + } + pgNames, err := readMigrationNames(pgMigrationsFS, "pgmigrations") + if err != nil { + t.Fatal(err) + } + for _, n := range pgNames { + if n == migration095 { + t.Fatalf("pgmigrations carries %s", n) + } + } + + s := testStore(t) + if s.dialect.Driver() != DriverPostgres { + return + } + var rel *string + if err := s.db.QueryRow(`SELECT to_regclass('items_fts')::text`).Scan(&rel); err != nil { + t.Fatal(err) + } + if rel != nil { + t.Fatalf("a migrated Postgres store has a relation named items_fts (%s)", *rel) + } +} + +// A notice keyed by a name no migration carries would never print, and the +// map would read as a claim that the slow migration announces itself. +func TestSlowSQLiteMigrationsNameRealMigrations(t *testing.T) { + t.Parallel() + names, err := readMigrationNames(migrationsFS, "migrations") + if err != nil { + t.Fatal(err) + } + have := map[string]bool{} + for _, n := range names { + have[n] = true + } + for n := range slowSQLiteMigrations { + if !have[n] { + t.Errorf("slowSQLiteMigrations names %q, which is not a migration", n) + } + } +} diff --git a/internal/store/migrations/095_rebuild_items_fts.sql b/internal/store/migrations/095_rebuild_items_fts.sql new file mode 100644 index 000000000..7dba20d06 --- /dev/null +++ b/internal/store/migrations/095_rebuild_items_fts.sql @@ -0,0 +1,66 @@ +-- Migration 095: restore items_fts and rebuild it once, to repair rows a +-- workspace import double-indexed (BUG-2758). +-- +-- items_fts is an EXTERNAL-CONTENT FTS5 table (content='items', +-- content_rowid='rowid') maintained by the items_fts_insert / _update / +-- _delete triggers (005_collections.sql, recreated verbatim by 056). Until +-- BUG-2758, ImportWorkspace and RemapAttachmentReferencesInWorkspace ran a +-- post-commit pass that inserted every imported item into items_fts a SECOND +-- time, on top of the trigger's insert (a third time when the bundle had +-- attachments). Every SQLite database that has ever imported a workspace +-- therefore holds duplicate postings, and FTS5's integrity-check reports the +-- table malformed. +-- +-- The migration RECREATES the index rather than repairing it in place, so +-- the state it leaves does not depend on the state it meets. Everything in +-- items_fts is derived from items, so dropping it loses nothing: +-- - the table is dropped and created with 005's definition, verbatim. A +-- damaged database may lack it, or hold one with another definition (a +-- contentless variant, other columns), and a bare 'rebuild' fails the +-- migration, and so startup, on each of those; +-- - the three triggers are recreated with 005's bodies (dropping the table +-- does not drop them, and a database may have lost one), so the index is +-- maintained again from here on; +-- - 'rebuild', FTS5's own command for an external-content table, populates +-- the new index from items: exactly what the triggers would have built. +-- +-- Precedent: 056_items_jsonb_not_null.sql recreated the same three triggers +-- verbatim from 005 and rebuilt the index the same way. 005's definitions are +-- still current: the only later migrations to touch items_fts are 046 +-- (documents_fts only) and 056 (the triggers, verbatim), so recreating from +-- 005 regresses nothing. +-- +-- Cost: measured with the sqlite3 CLI at 1.0 s for 9,766 items and 10.5 s for +-- a synthetic 97,660 (see slowSQLiteMigrations in store.go, which logs a line +-- before this runs). It runs at startup, before the server listens. +-- +-- SQLite only by construction: this directory is the SQLite chain. +-- Postgres runs pgmigrations/, has no items_fts, and keeps search_vector as a +-- trigger-maintained column on items, which the removed pass never touched. + +DROP TABLE IF EXISTS items_fts; + +CREATE VIRTUAL TABLE items_fts USING fts5( + title, content, tags, + content='items', + content_rowid='rowid' +); + +DROP TRIGGER IF EXISTS items_fts_insert; +DROP TRIGGER IF EXISTS items_fts_update; +DROP TRIGGER IF EXISTS items_fts_delete; + +CREATE TRIGGER items_fts_insert AFTER INSERT ON items BEGIN + INSERT INTO items_fts(rowid, title, content, tags) VALUES (NEW.rowid, NEW.title, NEW.content, NEW.tags); +END; + +CREATE TRIGGER items_fts_update AFTER UPDATE ON items BEGIN + INSERT INTO items_fts(items_fts, rowid, title, content, tags) VALUES('delete', OLD.rowid, OLD.title, OLD.content, OLD.tags); + INSERT INTO items_fts(rowid, title, content, tags) VALUES (NEW.rowid, NEW.title, NEW.content, NEW.tags); +END; + +CREATE TRIGGER items_fts_delete AFTER DELETE ON items BEGIN + INSERT INTO items_fts(items_fts, rowid, title, content, tags) VALUES('delete', OLD.rowid, OLD.title, OLD.content, OLD.tags); +END; + +INSERT INTO items_fts(items_fts) VALUES('rebuild'); diff --git a/internal/store/store.go b/internal/store/store.go index 0be397cf8..5ed2962cf 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -559,6 +559,17 @@ func readMigrationNames(fsys embed.FS, dir string) ([]string, error) { return names, nil } +// slowSQLiteMigrations names the SQLite migrations whose cost grows with the +// database, so the server logs a line before starting one and a slow first +// boot after an upgrade has an explanation in the log. The runner applies +// migrations under the database's write lock with no other output. +var slowSQLiteMigrations = map[string]string{ + // BUG-2758. Measured with the sqlite3 CLI: 1.0 s for 9,766 items (a copy of + // a 321 MB working database); 10.5 s for a synthetic 97,660 items + // (235 MB of content). Roughly linear in item content. + "095_rebuild_items_fts.sql": "rebuilding the item search index; this takes about 1 s per 10,000 items", +} + func (s *Store) migrate() error { // Create migrations tracking table _, err := s.db.Exec(`CREATE TABLE IF NOT EXISTS schema_migrations ( @@ -625,6 +636,9 @@ func (s *Store) migrate() error { return fmt.Errorf("read migration %s: %w", name, err) } + if notice, ok := slowSQLiteMigrations[name]; ok { + slog.Info("applying a slow migration", "migration", name, "note", notice) + } if err := applySQLiteMigration(s.db, name, string(data)); err != nil { return err }