From f72b3310a840ad5f97834b3a2743e597e282ad57 Mon Sep 17 00:00:00 2001 From: "shoney.arickathil" Date: Thu, 27 Aug 2026 16:33:36 +0200 Subject: [PATCH] =?UTF-8?q?refactor(db):=20wo=5Frow=5Fborrow/wo=5Frow=5Fre?= =?UTF-8?q?lease=20=E2=80=94=20one=20read=20path=20for=20both=20residencie?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task 5c step 1 of docs/superpowers/plans/2026-08-26-table-residency.md, as a PURE REFACTOR: no storage change, no keys-table anywhere. Borrow is wo_row_ptr plus a seam, every release is a no-op. Provable on its own before the storage change it exists to enable. DESIGN SETTLED BY READING THE STRUCTURES, and both answers make 5c smaller: - the id hash needs NO new storage. `hvals` is already uint64 holding slot+1 with 0 = empty (table.h), so offset+1 fits the same field, and the interpretation is per-table because a table is wholly `all` or wholly `keys`. No parallel map - secondary indexes need NO change. `db_ibucket.ids` stores row IDS, not slot indices, and table.c resolves them through the id hash. I had told the developer these pointed at slab slots — that was wrong, and it is why this is one shared accessor rather than 11 rewrites - the real coupling is the unique shadow: idx_add_row and row_apply_field_slot both FETCH the conflicting row and compare columns. Both now borrow/release, so a keys-table's non-resident conflict will be found rather than silently skipped — a unique check that only examines resident rows is a correctness hole, not a limitation The scratch lives on `db_table`, not on the stack and not per call. Per call would allocate once per candidate inside a bucket loop, turning an O(1) probe into an allocation storm; a stack buffer is unsafe because the loader bounds field_cnt at 65535 (loader.c:189), so the worst case is ~512 KB. It is safe per-table because the store is single-writer, and a `busy` flag is there to catch a nested borrow rather than let it alias silently. Freed in table_destroy. Gates: all 18 runtime suites 0 fail under ASan+UBSan (test_table 856/0, test_wal 3654/0), oop-e2e 119/0, residency 8/0, employee 8/0, db-actor 8/0. And the pure-refactor proof the plan asked for: `db-bench --quick` 85/0, every resident read/query/seed/write floor held — a refactor that moves a number is not a refactor. Remaining wo_row_ptr sites for 5d: 6 in table.c, 2 in db.c, 2 in wal.c. Co-Authored-By: Claude Opus 5 (1M context) --- database/src/table.c | 41 ++++++++- database/src/table.h | 30 +++++++ .../plans/2026-08-26-table-residency.md | 88 ++++++++++++++++++- 3 files changed, 151 insertions(+), 8 deletions(-) diff --git a/database/src/table.c b/database/src/table.c index 9cc18bb..183353f 100644 --- a/database/src/table.c +++ b/database/src/table.c @@ -449,8 +449,15 @@ static int idx_add_row(wo_db *db, db_table *t, db_row *r) { db_ibucket *b = idx_bucket(ix, idx_hash(c, ix, r), 0); if (!b) continue; for (uint32_t i = 0; i < b->len; i++) { - db_row *other = wo_row_ptr(db, t->class_id, b->ids[i]); - if (other && idx_cols_equal(c, ix, r, other)) return DB_ERR_UNIQUE; + /* databasev2 2: borrow, never peek at a slab. For a keys-table the + * conflicting row may not be resident, and a unique check that + * silently skipped non-resident rows would be a correctness hole, + * not a limitation. */ + const char *bmsg = ""; + db_row *other = wo_row_borrow(db, t->class_id, b->ids[i], &bmsg); + int clash = other && idx_cols_equal(c, ix, r, other); + wo_row_release(db, t->class_id, other); + if (clash) return DB_ERR_UNIQUE; } } for (uint32_t x = 0; x < t->index_cnt; x++) { @@ -498,6 +505,9 @@ int wo_db_init(wo_db *db, const wo_classdesc *classes, uint32_t class_cnt, } static void table_destroy(wo_db *db, db_table *t) { + free(t->scratch); /* databasev2 2 */ + t->scratch = NULL; + t->scratch_cap = 0; /* free every live row's engine-owned values, then the slabs */ const wo_classdesc *c = &db->classes[t->class_id]; for (uint32_t s = 0; s < t->slab_cnt; s++) { @@ -718,6 +728,26 @@ db_row *wo_row_ptr(wo_db *db, uint32_t class_id, uint64_t id) { return slot_row(t, (uint32_t)(s1 - 1)); } +db_row *wo_row_borrow(wo_db *db, uint32_t class_id, uint64_t id, const char **msg) { + (void)msg; + /* databasev2 2: only the resident backing exists so far. When + * `resident: keys` storage lands, this is where hvals is read as an + * OFFSET (it is already a uint64 holding slot+1, so offset+1 fits the + * same field) and 5b's wo_wal_read_row_at fills the table's scratch. + * Keeping the seam here, unused, is what makes that a local change + * instead of another sweep of every reader. */ + return wo_row_ptr(db, class_id, id); +} + +void wo_row_release(wo_db *db, uint32_t class_id, db_row *r) { + if (!r || class_id >= db->class_cnt) return; + db_table *t = &db->tables[class_id]; + if (!t->scratch_busy || (uint8_t *)r != t->scratch) return; /* slab-backed */ + const wo_classdesc *c = &db->classes[class_id]; + for (uint32_t i = 0; i < c->field_cnt; i++) wo_db_val_free(db, c->kinds[i], r->slots[i]); + t->scratch_busy = 0; +} + int wo_row_read(wo_db *db, wo_rt *rt, uint32_t class_id, uint64_t id, uint64_t *out_vals, const char **msg) { db_row *r = wo_row_ptr(db, class_id, id); @@ -903,8 +933,11 @@ static int row_apply_field_slot(wo_db *db, db_table *t, const wo_classdesc *c, if (!b) continue; for (uint32_t i = 0; i < b->len; i++) { if (b->ids[i] == id) continue; - db_row *other = wo_row_ptr(db, class_id, b->ids[i]); - if (other && idx_cols_equal(c, ix, r, other)) { + const char *bmsg = ""; + db_row *other = wo_row_borrow(db, class_id, b->ids[i], &bmsg); + int clash = other && idx_cols_equal(c, ix, r, other); + wo_row_release(db, class_id, other); + if (clash) { r->slots[field] = old; /* untouched, promised */ db_val_free(c->kinds[field], nv); if (err_kind) *err_kind = DB_ERR_UNIQUE; diff --git a/database/src/table.h b/database/src/table.h index 2e1210a..bbde56d 100644 --- a/database/src/table.h +++ b/database/src/table.h @@ -120,6 +120,16 @@ typedef struct db_table { /* secondary indexes, from the class table's v3 metadata */ db_index *indexes; uint32_t index_cnt; + /* databasev2 2: one reusable materialisation buffer per table, for + * wo_row_borrow. Per-TABLE and not per-call because the unique shadow + * check borrows once per candidate inside a bucket loop, and per-call + * allocation would turn an O(1) probe into an allocation storm. Safe + * because the store is single-writer (the owner shard) and a borrow is + * never nested — `busy` exists to catch it if that ever stops being + * true, rather than aliasing silently. */ + uint8_t *scratch; + size_t scratch_cap; + int scratch_busy; } db_table; typedef struct wo_db { @@ -171,6 +181,26 @@ int wo_row_update_field(wo_db *db, uint32_t class_id, uint64_t id, uint32_t fiel * to the VM. */ db_row *wo_row_ptr(wo_db *db, uint32_t class_id, uint64_t id); +/* ---- databasev2 2: the shared row accessor ------------------------------- + * + * Every reader that today does `wo_row_ptr` and then touches `r->slots[...]` + * uses this pair instead, so ONE code path serves both residencies: + * + * resident: all borrow returns the slab pointer; release is a no-op + * resident: keys borrow materialises the record from its log offset into + * the table's scratch; release frees what it built + * + * Landed as a PURE REFACTOR: until the offset storage exists, borrow is + * wo_row_ptr plus a branch and every release is a no-op. Deliberate — the + * refactor is provable on its own, before the storage change it enables. + * + * A borrowed row is READ-ONLY when it is materialised: it is a copy, so + * writing to it changes nothing durable. Mutation still goes through the row + * choke points. Pair EVERY non-NULL borrow with a release, and never nest two + * borrows on the same table — they would share one scratch. */ +db_row *wo_row_borrow(wo_db *db, uint32_t class_id, uint64_t id, const char **msg); +void wo_row_release(wo_db *db, uint32_t class_id, db_row *r); + /* Engine-internal, for WAL replay only: create a row with a FIXED id, * slots zeroed — the caller (wal.c) fills them with engine-encoded values * it built while decoding. Advances the table's next_id past [id] when the diff --git a/docs/superpowers/plans/2026-08-26-table-residency.md b/docs/superpowers/plans/2026-08-26-table-residency.md index 361e0fa..29d3efe 100644 --- a/docs/superpowers/plans/2026-08-26-table-residency.md +++ b/docs/superpowers/plans/2026-08-26-table-residency.md @@ -222,11 +222,14 @@ create fixtures under `tests/corpus/run/`. > | --- | --- | --- | > | **5a** | `wo_wal_next_offset` — exact record offsets, unit-proven | ✅ landed `ac7d8af` | > | **5b** | `wo_wal_read_row_at` — materialise a row from an offset into VM values. **Zero storage change**, so it is additive and independently testable | this section | -> | **5c** | the id→offset map for `resident: keys` classes, plus the missing drop-payload-keep-index operation | not started | -> | **5d** | rewiring `wo_row_ptr`'s call sites, the slab scans, and `@unique`/FK across the residency boundary | not started | +> | **5c** | the shared borrow/release accessor, then id→offset storage. **Design settled 2026-08-27** — see its section | written up | +> | **5d** | rewiring the readers: remaining call sites, slab scans, FK restrict, `@unique` across the boundary | scope recorded, write-up waits on 5c | > -> Only 5b is described below. 5c and 5d need their own task write-ups once 5b -> has shown what the read path actually costs. +> 5b and 5c are described below. **One correction:** an earlier version of +> this note said the secondary indexes point at slab slots. They store row +> **ids** (`table.h:88`) and are already indirect through the id hash, so they +> need no change — which is why 5c is one shared accessor rather than 11 +> rewrites. > **Retraction, 2026-08-26.** This plan originally had a Task 5 that rewrote @@ -292,6 +295,83 @@ slab path), `database/src/db.c` (insert/read/update/delete), `database/src/wal.c unchanged, since nothing calls the new function yet. - [ ] Commit. Draft: `feat(db): wo_wal_read_row_at — materialise a row from a log offset`. + +### 5c — the shared row accessor, then the offset map + +**Design settled 2026-08-27 by reading the structures rather than guessing. +Both open questions have answers, and both make this smaller than feared:** + +- **The id hash needs no new storage.** `db_table.hvals` is already `uint64_t` + holding *global slot + 1*, with 0 meaning empty (`table.h:117-119`). An + offset fits the same field, `offset + 1` reusing the same 0-is-empty trick. + The interpretation is per-table and decided by the class flag, because a + table is wholly `all` or wholly `keys` — never mixed. **No parallel map.** +- **Secondary indexes need no change at all.** `db_ibucket.ids` stores row + **ids**, not slot indices (`table.h:88`, and `table.c:452` resolves them via + `wo_row_ptr`). Every index is therefore already indirect through the id hash. + An earlier note in this plan — and an analogy given to the developer — said + these pointed at slots. That was wrong. +- **The unique shadow is the real coupling.** `idx_add_row` fetches the *other* + row and compares columns (`table.c:452-453`), as does + `row_apply_field_slot`'s update path. Those are the sites that need a row + they cannot get from a slab. + +**So the shape is one shared accessor, not 11 rewrites.** Every site that today +does `wo_row_ptr` then reads `r->slots[...]` becomes a borrow/release pair that +serves both modes: for `resident: all` it hands back the slab pointer and +releasing is a no-op; for `resident: keys` it materialises the record into +caller-provided scratch via 5b's `wo_wal_read_row_at` and releasing frees the +engine-owned values. One code path, two backings. + +**Files:** modify `database/src/table.h` / `table.c` (the accessor, then the +`hvals` interpretation), `database/src/db.c` (the insert path's drop-payload +step); extend `runtime/test/test_table.c`. + +**Interfaces:** +- Consumes: Task 3's class flags, 5a's `wo_wal_next_offset`, 5b's + `wo_wal_read_row_at`. +- Produces: `wo_row_borrow` / `wo_row_release`, which 5d rewires every + `wo_row_ptr` call site onto. + +- [ ] Add `wo_row_borrow(db, cid, id, scratch, msg)` returning a `db_row *`, + and `wo_row_release(db, cid, row, scratch)`. For a fully-resident table the + borrow is exactly today's `wo_row_ptr` and the release does nothing, so the + hot path gains at most a branch. Prove that first, alone, with **no + keys-table anywhere** — this step must be a pure refactor. +- [ ] Size the scratch honestly: a borrow needs `row_size` bytes plus the + engine-owned values its slots point at. Decide whether the caller supplies a + stack buffer sized from `row_size` or the accessor allocates; the unique + check runs inside a loop over a bucket, so an allocation per candidate would + turn an O(1) probe into an allocation storm. +- [ ] Verify: `make -C runtime test` and `test-iso` unchanged, `just oop-e2e`, + `just employee`, `just db-actor`, `just residency` unchanged, and `just + db-bench --quick` shows the resident read path inside its baseline tolerance. + A pure refactor that moves a number is not a pure refactor. +- [ ] Commit that refactor on its own, before any offset storage exists. +- [ ] Then: teach `hvals` the second interpretation, gated on the class flag — + `slot + 1` for `all`, `offset + 1` for `keys`. Keep the accessors for + reading it in one place so the two meanings cannot be confused at a call + site. +- [ ] Then: the insert path for a keys-table — apply to RAM (required, since + `enc_val` serialises *from* the slab), capture the offset, append, commit, + and only then drop the payload while leaving the id hash and every index + entry standing. This is the operation that does not exist today; + `wo_row_remove` also unhooks the indexes, so it cannot be reused. +- [ ] Verify: a keys-table insert leaves the id hash and indexes populated, the + slab slot recycled, and `wo_row_borrow` able to return the row from its + offset. ASan clean — the drop path frees engine values that the record now + owns instead. +- [ ] Commit. Draft: `feat(db): id->offset storage for resident:keys tables`. + +### 5d — rewire the readers + +Deliberately not written up until 5c's accessor exists, because its shape +decides how much of this is mechanical. Known scope: the remaining +`wo_row_ptr` call sites, the slab scans at `db.c:105-181` (a keys-table scan +walks the log sequentially instead), FK restrict's referrer scan, and the +`@unique` shadow across the boundary — the correctness core, since a +constraint that silently checks only resident rows must never ship. + ## Task 6 — the two runtime refusals **Files:** modify `runtime/src/main.c` (the `WO_DATA` block at :199-215),