fix+refactor: soft keywords as identifiers, full test wiring, ORC crash docs
- parser: clause keywords (header, format, status, user, csv, ...) now work
as identifiers everywhere; IMPORT/EXPORT accept FORMAT csv/HEADER true
- nimble test + CI run all 13 test suites (650 checks green)
- ExecutionContext.registry is now {.cursor.} (breaks registry<->ctx cycle)
- ORC crash reproduced and bisected (tests/orc_repro.py); ARC stays the MM
This commit is contained in:
@@ -0,0 +1,132 @@
|
||||
# BaraDB Stability Hardening — Design & Findings
|
||||
|
||||
Date: 2026-07-30
|
||||
Status: Implemented (phase A). Phases B/C proposed, awaiting decision.
|
||||
|
||||
## Goal
|
||||
|
||||
"Make the database better" — chosen direction: **stability first**. Establish a
|
||||
verified baseline, fix what is actually broken, and make the whole test suite
|
||||
run with one command before attempting any large refactoring or new features.
|
||||
|
||||
## Baseline established
|
||||
|
||||
- Debug build passes cleanly on Nim 2.2.10 (`nimble build_debug`).
|
||||
- The `hunos` build failure from `HUNOS_ISSUE.md` is **no longer an issue**:
|
||||
hunos 1.3.3 is installed and contains the `urandom` fix. The nimble file
|
||||
already allows `>= 1.3.0`.
|
||||
- Of the 13 test files in `tests/`, only `test_all` (+ `stress_test` in CI)
|
||||
ran automatically. Baseline run of the other 11: 10 pass,
|
||||
`nimforum_smoke_test` fails.
|
||||
|
||||
## Changes implemented
|
||||
|
||||
### 1. Parser: `header` usable as a column name (bug fix)
|
||||
|
||||
**Root cause.** `header` is a keyword token (`tkHeader`) used by
|
||||
`IMPORT/EXPORT ... HEADER`. The parser never accepted it as an identifier, so
|
||||
any table with a column named `header` (e.g. the nimforum schema) failed with
|
||||
`Expected identifier but got tkHeader`.
|
||||
|
||||
**Fix** (`src/barabadb/query/parser.nim`):
|
||||
- Added `tkHeader` to `identLikeKinds` (soft-keyword set used by
|
||||
`expectIdent`, line 38).
|
||||
- Added `tkHeader` to the identifier branch of `parsePrimary` (line 84) so
|
||||
`SELECT header ...` and `WHERE header = ...` work.
|
||||
- Dotted-path parsing (`a.b.c`) now uses `expectIdent` instead of
|
||||
`expect(tkIdent)` so `post.header` works too.
|
||||
|
||||
IMPORT/EXPORT parsing is unaffected: statement dispatch keys on the leading
|
||||
`tkImport`/`tkExport` and the clause parser peeks for `tkHeader` explicitly.
|
||||
|
||||
**Regression tests** (`tests/bugfix_test.nim`, new suite):
|
||||
- CREATE TABLE / INSERT / SELECT / WHERE with a column named `header`.
|
||||
- `IMPORT FROM ... HEADER no` and `EXPORT TO ... HEADER yes` still parse.
|
||||
|
||||
Note: `IMPORT ... FORMAT csv` currently fails because `csv` is also a keyword
|
||||
(`tkCsv`) and `parseImportFrom` expects `tkIdent` after `FORMAT`. Pre-existing
|
||||
limitation, **not** addressed here (out of scope; recorded for phase B).
|
||||
|
||||
### 2. All 13 test files wired into `nimble test` and CI
|
||||
|
||||
- `baradadb.nimble` `test` task now builds `build/baradadb` (the smoke test
|
||||
talks to it over TCP) and runs all 13 suites: quick embedded suites first,
|
||||
fuzz/property/stress last.
|
||||
- `.github/workflows/ci.yml` runs `nimble test` (was: only `test_all`);
|
||||
the now-redundant separate stress-test steps were removed.
|
||||
- Verified locally: full `nimble test` exits 0 with 648 passing checks.
|
||||
|
||||
### 2b. Soft-keyword cleanup (phase B3, implemented)
|
||||
|
||||
Extended the `header` approach to all clause-only keywords, so they work as
|
||||
table/column names everywhere (DDL, DML, aliases, dotted paths, CTEs, JOINs,
|
||||
MERGE, GRANT/REVOKE, SET):
|
||||
`format, delimiter, batch, csv, ndjson, status, migration, apply, up, down,
|
||||
dryrun, user, policy, enable, disable, recover, before, after, instead, of`.
|
||||
|
||||
- `identLikeKinds` and the `parsePrimary` identifier branch now include them.
|
||||
- All 69 `expect(tkIdent)` call sites now use `expectIdent` — a strict
|
||||
superset, so previously valid SQL is unaffected (verified: full suite green).
|
||||
- `IMPORT/EXPORT`: `FORMAT csv/ndjson/json` and `HEADER true/false` now parse
|
||||
(previously `csv`/`true`/`false` lexed as keywords and were rejected despite
|
||||
the grammar clearly intending them). Clause table names use `expectIdent`.
|
||||
|
||||
Structural keywords (`where`, `group`, `order`, `join`, `on`, `for`, `using`,
|
||||
`view`, `trigger`, `import`, `export`, `grant`, ...) remain reserved.
|
||||
|
||||
**Regression tests** (`tests/bugfix_test.nim`): a table with 21 keyword-named
|
||||
columns through CREATE/INSERT/UPDATE/SELECT/qualified refs, plus
|
||||
IMPORT/EXPORT keyword-value parsing.
|
||||
|
||||
### 3. ORC crash — reproduced, bisected, three root-cause attempts failed
|
||||
|
||||
`nim.cfg` forces `--mm:arc` because ORC's cycle collector crashed
|
||||
("markGray/trace SIGSEGV after ~20 sequential INSERTs"). The ARC cycle-breaking
|
||||
in commit `ed5a719` did not fix the ORC path.
|
||||
|
||||
**Reproduction** (`tests/orc_repro.py`): build the server with `--mm:orc`,
|
||||
drive 1000 sequential TCP INSERTs plus 10 concurrent connections. The server
|
||||
dies with the exact documented signature (`handleClient` →
|
||||
`nimDecRefIsLastCyclicStatic` → `collectCyclesBacon` → `markGray` → `trace` →
|
||||
SIGSEGV).
|
||||
|
||||
**Bisect:** 200 pings + 200 SELECTs over TCP are fine; the crash lands between
|
||||
20 and 500 sequential INSERTs — INSERT path only.
|
||||
|
||||
**Failed root-cause attempts:**
|
||||
1. `ed5a719` — callback cycle breaks (shard/gossip).
|
||||
2. `{.cursor.}` on `ExecutionContext.registry` — breaks the real
|
||||
`DatabaseRegistry ↔ ExecutionContext` cycle (kept: it is the correct
|
||||
ownership annotation regardless), but the crash persists unchanged.
|
||||
3. Guarding `ctx.onChange` against zero WS subscribers (reverted: fixed
|
||||
nothing).
|
||||
|
||||
**Conclusion:** per the 3-strikes rule this is a deep ORC+async issue —
|
||||
possibly an upstream Nim 2.2.x ORC bug with async closure environments and/or
|
||||
complex generic types — not a single app-level cycle. ARC remains the
|
||||
supported memory manager (full suite green under it). The findings are
|
||||
recorded in `nim.cfg` and `tests/orc_repro.py` for a future attempt (e.g.
|
||||
re-test with a newer Nim runtime, or a minimal repro filed upstream).
|
||||
|
||||
## Proposed next phases (not started)
|
||||
|
||||
- **B2. Split `query/executor.nim`** (5,398 lines) into focused modules
|
||||
(DML, DDL, select pipeline, transactions). Now safe to do: the full suite
|
||||
guards behavior. Large diff, mechanical.
|
||||
- **C. Features** — real Raft network transport, persistence for
|
||||
graph/FTS/columnar engines, benchmark validation.
|
||||
- **ORC (blocked):** re-test `tests/orc_repro.py` against a newer Nim runtime;
|
||||
if it persists, distill a minimal repro and file upstream. Not app-actionable
|
||||
today (see "ORC crash" section).
|
||||
|
||||
## Verification evidence
|
||||
|
||||
- `nimble test` (all 13 suites): exit 0, 650 `[OK]`, 0 failed — final run
|
||||
after all changes (phase A + B3 + cursor).
|
||||
- `nimforum_smoke_test` (rebuilt server): all suites `[OK]`, including
|
||||
`NimForum schema creation` (previously `[FAILED]`).
|
||||
- B3 TDD: new keyword tests failed first with the expected `tkFormat`/`tkCsv`
|
||||
errors, then passed; `test_all` stayed green (461 `[OK]`).
|
||||
- ORC investigation: embedded `test_wire_insert_stress` passes even when
|
||||
compiled with `--mm:orc`; the TCP **server** compiled with `--mm:orc`
|
||||
crashes as documented above. All shipped artifacts use ARC and are green.
|
||||
Reference in New Issue
Block a user