diff --git a/docs/superpowers/plans/2026-07-30-engine-persistence.md b/docs/superpowers/plans/2026-07-30-engine-persistence.md new file mode 100644 index 0000000..3b6d294 --- /dev/null +++ b/docs/superpowers/plans/2026-07-30-engine-persistence.md @@ -0,0 +1,333 @@ +# Engine Persistence (C1) Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make FTS indexes, HNSW vector indexes, and graphs survive a database restart — same query results after reopen, live index updates — fixing the silent-empty-results correctness bug. + +**Architecture:** Follow the existing schema-durability pattern: engine DDL is persisted under new `_schema:` key prefixes in the LSM store; at startup a new `restoreEngines*(ctx)` (living in `executor.nim`, the top module) replays it — FTS/HNSW by re-parsing + re-executing the CREATE INDEX DDL via `executeQueryImpl` (rebuild-from-scan), graphs by rebuilding the `Graph` object from the backing `_nodes`/`_edges` tables with the same row→graph mapping the DML path uses (`exec/dml.nim:155-195`). `exec/context.nim` gets one hook var `restoreEnginesHook*` called at the end of `newExecutionContext`, wired by `executor.nim` (the established hook pattern — Nim forbids circular imports; context is L1, restore logic needs L5+ modules). + +**Tech Stack:** Nim 2.2.10, ARC, unittest. + +## Global Constraints + +- Spec: `docs/superpowers/specs/2026-07-30-engine-persistence-design.md` (read it first). +- Test command per task: `nim c -d:ssl --threads:on --path:src -o:tests/test_schema_persist tests/test_schema_persist.nim && ./tests/test_schema_persist` AND `nim c -d:ssl --threads:on --path:src -o:tests/test_all tests/test_all.nim && ./tests/test_all` — both exit 0. +- TDD: write the failing test FIRST in `tests/test_schema_persist.nim`, watch it fail for the right reason, then implement. +- No behavior changes outside the persistence semantics. No new dependencies. Pure additive `_schema:` keys — old databases without the keys start exactly as before. +- Public API freeze: no changes to exported signatures; additive procs only. +- Failures during engine restore must NOT prevent startup: per-key try/except, warn via the project's logging, continue. +- Reopen pattern for tests (existing in test_schema_persist.nim — reuse its helpers): build ctx on a temp dir, close, create a NEW LSMTree + ExecutionContext on the same dir, assert. +- Commits: per task, after tests pass (user approved per-task commits for this workflow). +- Relevant code facts (verified, use them): + - CREATE INDEX FTS branch: `src/barabadb/query/executor.nim:1285-1300`; HNSW: `1302-1334`; B-tree: `1336-1350`. AST fields: `stmt.ciKind` (`ikFullText`/`ikHNSW`), `stmt.ciName`, `stmt.ciTarget`, `stmt.ciColumns`. + - DROP INDEX: `executor.nim:1352-1370` — currently only searches `ctx.btrees`; FTS/HNSW indexes cannot be dropped at all today. + - CREATE GRAPH: `executor.nim:882-905` (fails if backing tables exist — hence the loader approach, not replay); DROP GRAPH: `907-922`. + - DROP TABLE: `executor.nim:854-880` (deletes btrees + dropTableSchema + data keys; engine cleanup must be added here). + - Graph row→object mapping to mirror in the loader: `exec/dml.nim:155-195` (`addNodeWithId` with props from non-id/node_label/properties columns; `addEdgeWithId` with parsed weight). + - Graph engine API: `gengine.newGraph/addNodeWithId/addEdgeWithId` (`graph/engine.nim:51,71,99`). + - `newExecutionContext` calls `restoreSchema` at `exec/context.nim:41`; hook call goes right after. + - Hook idiom to copy: `exec/eval.nim` (var + require* nil-guard) and the wiring block at the bottom of `executor.nim`. + - Restore must replay via `executeQueryImpl` (NO DDL lock — the lock lives only in the `executeQuery` wrapper; verify). + +--- + +### Task 1: Hook scaffold + FTS index persistence + +**Files:** +- Modify: `src/barabadb/query/exec/context.nim` (hook var + call in newExecutionContext) +- Modify: `src/barabadb/query/executor.nim` (persist key in FTS branch, restoreEngines proc, wiring) +- Modify: `src/barabadb/query/exec/schema.nim` (new key prefix const, if the pattern is followed there) +- Test: `tests/test_schema_persist.nim` + +**Interfaces:** +- Consumes: existing `restoreSchema` flow; `execScan` for rebuild. +- Produces: + - `var restoreEnginesHook*: proc(ctx: ExecutionContext)` in `exec/context.nim`, called at the end of `newExecutionContext` (nil-safe: `if restoreEnginesHook != nil: restoreEnginesHook(result)`). + - `proc restoreEngines*(ctx: ExecutionContext)` in `executor.nim` — wired via `context.restoreEnginesHook = restoreEngines` in the existing hook-wiring block at module scope. + - Key format: `_schema:ftsidx:.` → reconstructed DDL `CREATE INDEX ON
() USING FTS` (col list joined; ciName fallback to colKey when empty, mirroring executor.nim:1283). + +- [ ] **Step 1: Write the failing test** + +Add to `tests/test_schema_persist.nim` (match existing suite style): + +```nim + test "FTS index survives reopen": + let dir = testDir & "_fts" + removeDir(dir) + createDir(dir) + block: + let db = newLSMTree(dir) + var ctx = newExecutionContext(db) + discard executeQuery(ctx, parse("CREATE TABLE docs (id INTEGER PRIMARY KEY, content TEXT)")) + discard executeQuery(ctx, parse("INSERT INTO docs (id, content) VALUES (1, 'quick brown fox')")) + discard executeQuery(ctx, parse("CREATE INDEX docs_fts ON docs (content) USING FTS")) + ctx.db.close() + block: + let db = newLSMTree(dir) + var ctx = newExecutionContext(db) + let r = executeQuery(ctx, parse("SELECT hybrid_search_ids('docs', 'content', 'quick') AS ids")) + check r.success + check valueToString(r.rows[0]["ids"]).contains("docs.1") + # index keeps updating after reopen + discard executeQuery(ctx, parse("INSERT INTO docs (id, content) VALUES (2, 'quick red fox')")) + let r2 = executeQuery(ctx, parse("SELECT hybrid_search_ids('docs', 'content', 'red') AS ids")) + check r2.success + check valueToString(r2.rows[0]["ids"]).contains("docs.2") + ctx.db.close() + removeDir(dir) +``` + +(If `hybrid_search_ids` signature differs, copy the exact working call from `tests/test_all.nim`'s Hybrid RAG Search suite. The key assertion: results are non-empty after reopen — today they are silently empty.) + +- [ ] **Step 2: Run it, watch it fail** + +Run: `nim c -d:ssl --threads:on --path:src -o:tests/test_schema_persist tests/test_schema_persist.nim && ./tests/test_schema_persist` +Expected: FAIL — ids string empty (index vanished after reopen). + +- [ ] **Step 3: Implement** + +1. `exec/context.nim`: add `var restoreEnginesHook*: proc(ctx: ExecutionContext)` with a doc comment (wired by executor.nim; breaks the module layering cycle), and call it nil-safely at the end of `newExecutionContext`. +2. `executor.nim`, FTS branch (after `ctx.ftsIndexes[colKey] = ftsIdx`, before the return): persist the reconstructed DDL: + ```nim + let ftsDdl = "CREATE INDEX " & idxName & " ON " & stmt.ciTarget & " (" & stmt.ciColumns.join(", ") & ") USING FTS" + ctx.db.put("_schema:ftsidx:" & colKey, cast[seq[byte]](ftsDdl)) + ``` +3. `executor.nim`: new `proc restoreEngines*(ctx: ExecutionContext)` — scans `ctx.db.scanAll()` for keys starting with `_schema:ftsidx:`, per key: try `executeQueryImpl(ctx, qpar.parse(qlex.tokenize(cast[string](value))))` (log warning + continue on failure; replay re-persists the same key idempotently). Wire `context.restoreEnginesHook = restoreEngines` in the module-scope hook block. + +- [ ] **Step 4: Run tests, watch them pass** + +Run both test commands from Global Constraints. +Expected: new test PASS; test_all exit 0, 461+ `[OK]`. + +- [ ] **Step 5: Commit** + +```bash +git add src/barabadb/query/exec/context.nim src/barabadb/query/executor.nim tests/test_schema_persist.nim +git commit -m "feat(persist): FTS indexes survive restart (schema key + restore replay)" +``` + +--- + +### Task 2: HNSW vector index persistence + +**Files:** +- Modify: `src/barabadb/query/executor.nim` (persist key in HNSW branch; extend restoreEngines scan) +- Test: `tests/test_schema_persist.nim` + +**Interfaces:** +- Consumes: Task 1's restoreEngines + hook. +- Produces: key format `_schema:vecidx:
.` → `CREATE INDEX ON
() USING HNSW`; restoreEngines scans both prefixes. + +- [ ] **Step 1: Write the failing test** + +```nim + test "HNSW vector index survives reopen": + let dir = testDir & "_vec" + removeDir(dir) + createDir(dir) + block: + let db = newLSMTree(dir) + var ctx = newExecutionContext(db) + discard executeQuery(ctx, parse("CREATE TABLE vecs (id INTEGER PRIMARY KEY, embedding TEXT)")) + discard executeQuery(ctx, parse("INSERT INTO vecs (id, embedding) VALUES (1, '[1.0, 0.0, 0.0]')")) + discard executeQuery(ctx, parse("CREATE INDEX vecs_hnsw ON vecs (embedding) USING HNSW")) + ctx.db.close() + block: + let db = newLSMTree(dir) + var ctx = newExecutionContext(db) + let r = executeQuery(ctx, parse("SELECT hybrid_search_ids('vecs', 'embedding', '', '[1.0, 0.0, 0.0]') AS ids")) + check r.success + check valueToString(r.rows[0]["ids"]).contains("vecs.1") + ctx.db.close() + removeDir(dir) +``` + +(If no pure-vector query form exists, copy the exact working vector-search call from test_all's Hybrid RAG suite — e.g. `hybrid_search_filtered` with a vector arg. Assertion: non-empty after reopen.) + +- [ ] **Step 2: Run it, watch it fail** + +Expected: FAIL — empty ids after reopen. + +- [ ] **Step 3: Implement** + +Mirror Task 1: persist `_schema:vecidx:` + reconstructed `USING HNSW` DDL in the HNSW branch; add the `_schema:vecidx:` prefix to the restoreEngines scan (same replay path). + +- [ ] **Step 4: Run tests, watch them pass** + +Both test commands; expected PASS + test_all green. + +- [ ] **Step 5: Commit** + +```bash +git add src/barabadb/query/executor.nim tests/test_schema_persist.nim +git commit -m "feat(persist): HNSW vector indexes survive restart" +``` + +--- + +### Task 3: Graph persistence + +**Files:** +- Modify: `src/barabadb/query/executor.nim` (persist marker on CREATE GRAPH, delete on DROP GRAPH, graph loader in restoreEngines) +- Test: `tests/test_schema_persist.nim` + +**Interfaces:** +- Consumes: Task 1's restoreEngines; graph engine API (`gengine.newGraph/addNodeWithId/addEdgeWithId`); row→graph mapping from `exec/dml.nim:155-195`. +- Produces: key format `_schema:graphs:` → original-ish DDL `CREATE GRAPH ` (marker + introspection); graph loader that rebuilds `ctx.graphs[name]` from `_nodes` / `_edges` rows. + +- [ ] **Step 1: Write the failing test** + +```nim + test "Graph survives reopen": + let dir = testDir & "_graph" + removeDir(dir) + createDir(dir) + block: + let db = newLSMTree(dir) + var ctx = newExecutionContext(db) + discard executeQuery(ctx, parse("CREATE GRAPH social")) + discard executeQuery(ctx, parse("INSERT INTO social_nodes (id, node_label) VALUES (1, 'person')")) + discard executeQuery(ctx, parse("INSERT INTO social_nodes (id, node_label) VALUES (2, 'person')")) + discard executeQuery(ctx, parse("INSERT INTO social_edges (source_id, dest_id, edge_label, weight) VALUES (1, 2, 'knows', 1.0)")) + ctx.db.close() + block: + let db = newLSMTree(dir) + var ctx = newExecutionContext(db) + check "social" in ctx.graphs + check gengine.nodeCount(ctx.graphs["social"]) == 2 + check gengine.edgeCount(ctx.graphs["social"]) == 1 + ctx.db.close() + removeDir(dir) +``` + +(Adjust imports: the test file needs `barabadb/graph/engine as gengine`. If a higher-level graph query is easily available from test_all's graph suites, prefer asserting on that instead/in addition.) + +- [ ] **Step 2: Run it, watch it fail** + +Expected: FAIL — `"social" in ctx.graphs` is false after reopen. + +- [ ] **Step 3: Implement** + +1. CREATE GRAPH branch (executor.nim:882-905): on success, `ctx.db.put("_schema:graphs:" & name, cast[seq[byte]]("CREATE GRAPH " & name))`. On the failure paths, no key is written. +2. DROP GRAPH branch (907-922): `ctx.db.delete("_schema:graphs:" & name)`. +3. restoreEngines: for each `_schema:graphs:` key — extract name; skip if already in `ctx.graphs`; build: + ```nim + var g = gengine.newGraph() + # mirror exec/dml.nim:155-195 mapping + for row in execScan(ctx, name & "_nodes"): + # id, node_label, props = all other columns except id/node_label/properties + ... + for row in execScan(ctx, name & "_edges"): + # source_id, dest_id, edge_label, weight (parseFloat, default 1.0) + ... + ctx.graphs[name] = g + ``` + Per-key try/except with warning + continue. Use `gengine.addNodeWithId` / `addEdgeWithId` with the SAME failure tolerance as dml.nim (except CatchableError: discard per row). + +- [ ] **Step 4: Run tests, watch them pass** + +Both test commands; expected PASS + test_all green. + +- [ ] **Step 5: Commit** + +```bash +git add src/barabadb/query/executor.nim tests/test_schema_persist.nim +git commit -m "feat(persist): graphs survive restart (rebuild from backing tables)" +``` + +--- + +### Task 4: DROP paths completeness + +**Files:** +- Modify: `src/barabadb/query/executor.nim` (DROP INDEX for FTS/HNSW, DROP TABLE engine cleanup) +- Test: `tests/test_schema_persist.nim` + +**Interfaces:** +- Consumes: Tasks 1-3 key formats. +- Produces: DROP INDEX removes in-memory FTS/HNSW index + its `_schema:` key; DROP TABLE removes engine indexes/keys for that table. + +- [ ] **Step 1: Write the failing tests** + +```nim + test "DROP INDEX removes FTS index and its schema key": + let dir = testDir & "_dropfts" + removeDir(dir) + createDir(dir) + block: + let db = newLSMTree(dir) + var ctx = newExecutionContext(db) + discard executeQuery(ctx, parse("CREATE TABLE docs (id INTEGER PRIMARY KEY, content TEXT)")) + discard executeQuery(ctx, parse("INSERT INTO docs (id, content) VALUES (1, 'quick brown fox')")) + discard executeQuery(ctx, parse("CREATE INDEX docs_fts ON docs (content) USING FTS")) + let d = executeQuery(ctx, parse("DROP INDEX docs_fts")) + check d.success + check "docs.content" notin ctx.ftsIndexes + ctx.db.close() + block: + let db = newLSMTree(dir) + var ctx = newExecutionContext(db) + check "docs.content" notin ctx.ftsIndexes # no ghost rebuild + ctx.db.close() + removeDir(dir) + + test "DROP TABLE removes engine indexes for that table": + # same flow without the DROP INDEX; DROP TABLE docs instead; + # after reopen, ctx.ftsIndexes must not contain docs.content + # and the _schema:ftsidx:docs.content key must be gone +``` + +(Write both fully, mirroring the first test's structure; adjust colKey format to the actual one — `table.col`.) + +- [ ] **Step 2: Run them, watch them fail** + +Expected: DROP INDEX test FAILS (FTS index can't be dropped today — `d.success` false or index still present / ghost rebuild after reopen). DROP TABLE test likely FAILS on the ghost-rebuild assertion. + +- [ ] **Step 3: Implement** + +1. DROP INDEX (executor.nim:1352-1370): before/after the btree search, also check `ctx.ftsIndexes` and `ctx.vectorIndexes` for a key == stmt.diName or ending with "." & stmt.diName or whose idxName matches (mirror the colKey/idxName convention from CREATE INDEX: idxName defaults to colKey); on hit: delete in-memory entry AND `ctx.db.delete("_schema:ftsidx:" / "_schema:vecidx:" & key)`. Keep the existing btree + `_schema:indexes:` behavior untouched. +2. DROP TABLE (executor.nim:854-880): alongside the btree sweep — delete `ctx.ftsIndexes`/`ctx.vectorIndexes` entries whose key starts with `dropName & "."`, and delete the corresponding `_schema:ftsidx:`/`_schema:vecidx:` keys (scan for prefix, same style as the data-keys sweep). + +- [ ] **Step 4: Run tests, watch them pass** + +Both test commands; expected PASS + test_all green. + +- [ ] **Step 5: Commit** + +```bash +git add src/barabadb/query/executor.nim tests/test_schema_persist.nim +git commit -m "fix: DROP INDEX/TABLE clean up FTS/HNSW indexes and their schema keys" +``` + +--- + +### Task 5: Full verification + docs + +**Files:** +- Modify: `docs/superpowers/specs/2026-07-30-engine-persistence-design.md` (status → done) +- Modify: README.md feature claims ONLY IF it explicitly says FTS/vector/graph persistence is missing/optional (check first; minimal edit) + +- [ ] **Step 1: Full suite** + +Run: `nimble test` +Expected: exit 0, 650+ `[OK]` (new tests add to the count), 0 failed. + +- [ ] **Step 2: Docs** + +Update the spec status line. Check README for "persistence optional"-style claims about graph/FTS (`grep -n -i 'persist' README.md docs/en/*.md | head -20`); update only lines that are now false, minimally. + +- [ ] **Step 3: Commit** + +```bash +git add docs/ README.md +git commit -m "docs: engine persistence (C1) done" +``` + +--- + +## Self-Review Notes + +- Spec coverage: §1 (persist DDL) → Tasks 1-3; §2 (restore) → Tasks 1-3 via restoreEngines; DROP paths → Task 4; testing § → each task's Step 1; failure tolerance → restoreEngines try/except; lock concern → replay via executeQueryImpl (lock lives in the executeQuery wrapper only). +- CREATE GRAPH replay rejected (fails on existing backing tables) → loader approach, per spec's "pick whichever is smaller" clause. +- Riskiest spot: the colKey/idxName conventions in DROP INDEX (Task 4) — tests pin them down. +- Type consistency: hook type `proc(ctx: ExecutionContext)` matches restoreEngines; copied hook idiom from exec/eval.nim. diff --git a/docs/superpowers/specs/2026-07-30-engine-persistence-design.md b/docs/superpowers/specs/2026-07-30-engine-persistence-design.md new file mode 100644 index 0000000..ab906fc --- /dev/null +++ b/docs/superpowers/specs/2026-07-30-engine-persistence-design.md @@ -0,0 +1,118 @@ +# Engine Persistence (C1) — Design + +Date: 2026-07-30 +Status: Approved direction (user: "продължи"); implementation follows. + +## Problem + +After a server restart, FTS indexes, HNSW vector indexes, and graph objects +silently vanish while the underlying rows survive (LSM+WAL): + +- `CREATE INDEX ... USING FTS` builds an in-memory `InvertedIndex` only + (`ctx.ftsIndexes`); the DDL is never persisted under `_schema:`. + DML keeps updating it in memory (`exec/dml.nim:81-89, 295-304`), but nothing + writes it down. +- Same for HNSW (`ctx.vectorIndexes`) and graphs (`ctx.graphs`; only the + backing `_nodes`/`_edges` tables are durable). +- `restoreSchema` (`exec/schema.nim:167-231`) restores tables, views, + triggers, users, policies and rebuilds B-tree secondary indexes — but not + these three engines. Their `CREATE` DDL is not even stored. +- User-visible effect: hybrid search and graph queries return **silently + empty results** after restart (guards in `exec/eval.nim:103,150,968-984`), + and new inserts no longer update the lost indexes. This is a correctness + bug, not a performance gap. + +## Goal + +FTS indexes, HNSW vector indexes, and graphs survive a restart: after +reopening the database, the same queries return the same results, and new +DML keeps the indexes up to date. + +Non-goals (later phases): snapshot-based persistence for fast startup (C2), +columnar persistence (no `ctx` integration exists yet), Raft transport (C3). + +## Design + +Follow the existing schema-durability pattern: DDL is serialized under +stable `_schema::` keys in the LSM store and replayed at startup +by `restoreSchema`. + +### 1. Persist engine DDL + +New key prefixes (mirroring `SchemaTablePrefix` etc. in `exec/schema.nim:14-20`): + +- `_schema:ftsidx:
.` → the original `CREATE INDEX ... USING FTS` DDL +- `_schema:vecidx:
.` → the original `CREATE INDEX ... USING HNSW/VECTOR` DDL +- `_schema:graphs:` → the original `CREATE GRAPH ...` DDL + +Written by the corresponding `CREATE` execution paths in the dispatcher +(`executeQueryImpl`), deleted by `DROP INDEX` / `DROP GRAPH`, and cleaned up +on `DROP TABLE` for keys belonging to that table (same sweep the current +code does for `_schema:tables:` and secondary-index metadata — check and +extend that path). + +### 2. Restore on startup + +`restoreSchema` runs after tables are restored and B-tree indexes rebuilt. +New step, in this order: + +1. Scan `_schema:ftsidx:` / `_schema:vecidx:` keys, parse each DDL, and + **re-execute it through `executeQuery`** (the same way table DDL is + re-applied). The CREATE INDEX path already builds the in-memory index + from a full table scan, so replay == rebuild, no new build logic. +2. Scan `_schema:graphs:` keys and re-execute the CREATE GRAPH DDL. + Caveat to resolve at implementation time: replaying CREATE GRAPH must + not fail when the backing `_nodes`/`_edges` tables already exist + (they were restored as regular tables). If the CREATE GRAPH execution + errors on existing tables, the restore path instead rebuilds the in-memory + `Graph` object by scanning those tables (small new loader in the graph + engine usage site — the engine has no table-scan loader today, only + unused binary file save/load). Pick whichever is smaller and matches + existing behavior; document the choice in the plan. + +Failures during engine restore must not prevent startup: log a warning and +continue (a corrupt engine DDL must not take the database down — matches +the defensive style of `restoreSchema`). + +### 3. Ordering constraints + +- Table restore + row data available BEFORE engine replay (indexes build + from scans; graphs build from `_nodes`/`_edges`). +- Engine replay runs before the context is served to connections + (it is part of `newExecutionContext` → `restoreSchema`). + +### 4. What does NOT change + +- In-memory update paths in `exec/dml.nim` (insert/update/delete keeping + indexes current) — untouched; they work once the indexes exist again. +- LSM/WAL mechanics; `_schema` table format; B-tree index rebuild. +- Public API: none. + +## Testing + +New suite in `tests/test_schema_persist.nim` (the existing persistence test +file), TDD — each test fails before the fix: + +1. FTS: create table, insert docs, `CREATE INDEX ... USING FTS`, close DB, + reopen, `hybrid_search`-style query / FTS MATCH query returns the doc + (pre-fix: silently empty). Insert another doc after reopen and verify it + is found too (index updates live again). +2. Vector: same flow with an HNSW index and a vector search query. +3. Graph: create graph, add nodes/edges, close, reopen, graph query returns + the traversal (pre-fix: `"[]"`). +4. DROP INDEX removes the `_schema:ftsidx:`/`:vecidx:` key (no ghost rebuild + after reopen). DROP TABLE removes engine keys for that table. +5. Existing persistence tests keep passing (regression). + +Verification gate: full `nimble test` green (currently 650 `[OK]`). + +## Risks + +- CREATE GRAPH replay semantics (see §2) — resolved during planning with a + concrete read of the dispatcher's graph DDL path. +- Startup cost: full-scan rebuilds are O(table size) per index, same as + today's B-tree rebuild. Acceptable for C1; C2 (snapshots) addresses it. +- Re-executing DDL through `executeQuery` inside `restoreSchema` — must not + deadlock on `ctx.sharedLock` (DDL lock is taken by `executeQuery`; + `restoreSchema` runs during context construction, before serving — verify + no lock is held at that point).