fix(store): a workspace import no longer re-inserts every item into items_fts after its commit, and migration 095 rebuilds the index (BUG-2758) (#1507)

ImportWorkspace and RemapAttachmentReferencesInWorkspace each ran a
post-commit rebuildFTSForWorkspace: one autocommit INSERT INTO items_fts per
item. items_fts is an external-content FTS5 table kept by the items_fts_*
triggers, which had already indexed those rows inside the transaction. So the
pass double-indexed every imported item (FTS5 integrity-check reports the
table malformed), and it turned an N-item import into N write transactions
after the commit, which stalled every other writer on the database. Under
e2e load that stall outlasted the 30 s busy_timeout, which is why
version-restore-collab and workspace-bundle-roundtrip failed together.

The pass and both call sites are removed; the triggers are the index.
Migration 095 (SQLite chain only) drops and recreates items_fts and its three
triggers with 005's definitions, then runs FTS5 'rebuild', so it repairs
existing databases whatever state they are in. slowSQLiteMigrations logs a
line before it starts.

Claude-Session: https://claude.ai/code/session_01Vf8uZmYdsrQqJtbAmjUFSX
This commit is contained in:
xarmian
2026-09-24 09:16:17 -04:00
committed by GitHub
parent f5b0062d6e
commit 7c2c514d72
6 changed files with 498 additions and 33 deletions
+9 -8
View File
@@ -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
+6 -25
View File
@@ -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.
//
+151
View File
@@ -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))
}
}
@@ -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)
}
}
}
@@ -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');
+14
View File
@@ -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
}