refactor(db): wo_row_borrow/wo_row_release — one read path for both residencies
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) <noreply@anthropic.com>
This commit is contained in:
parent
51dd42f9db
commit
f72b3310a8
3 changed files with 151 additions and 8 deletions
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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),
|
||||
|
|
|
|||
Loading…
Reference in a new issue