cache: unique-column lookups maintained from the schema #44

Merged

yuris merged 1 commit from feat/28-unique-index into main 2026-08-19 18:31:58 +00:00

Conversation 1 Commits 1 Files changed 4 +433 -20

yuris commented 2026-08-19 17:58:38 +00:00

Owner

Copy link

Closes #28.

cache.get looks up by primary key only; any other unique column needed a full cache.iter scan. Now that the generator emits unique, the cache maintains a lookup per unique column:

cache.find(state, "player", "identity", my_identity)

The motivating shape is the common one in a game: a player table keyed by auto-increment id with a unique identity column. "Which row is me?" runs on every connect and every reconnect, and it was O(n) on the main thread.

Design decisions

• state.indexes[table][column][value] = row, built once from the schema at cache.new, rebuilt by drop_all.
• The pk column is deliberately not mirrored. state.tables[name] already is that index; mirroring would double memory and leave two structures to keep in step.
• find on the pk column delegates to get, so callers — and future generated accessors — route every single-column unique lookup through one function without special-casing the pk. get stays the direct spelling. Both are frozen at 1.0, so the relationship is spelled out in find's docstring.
• Maintenance lives inside the private store / remove, which are only ever reached from apply_database_update. No separate pass, and #21's pairing stays purely an event concern — the index needs no second diff.
• find on a non-unique column raises, rather than returning nil or scanning. Returning nil would be indistinguishable from a genuine miss and would hide a lookup that can never succeed; a scan would be a silent O(n) on the main thread. A unique column with no match returns nil, so hit / miss / unusable are three distinguishable outcomes.
• schema.new now rejects a unique name that is not a column, for the same reason it already rejects such a pk: it would build an index no row can populate, making find miss forever instead of saying why.

The correctness case that matters

A unique column's value changing must erase the old key before writing the new one, or the index keeps a stale entry pointing at a row that no longer holds that value.

That falls out of the existing delete-then-insert ordering — the delete half carries the old row, so the old key is erased before the insert writes the new one. The case ordering does not cover is a row change arriving as a bare insert with no delete half, which the pk store has always treated as an upsert; store therefore explicitly erases the keys of the row it replaces. Separately tested.

reindex's old side is always the row the cache is holding, never a row straight off the wire, so an entry can only ever be erased by the row that owns it.

Tables with no unique metadata get no index entry and pay one nil check. Hand-written schemas without a unique field work unchanged.

Verification

5 passed, 0 failed; stylua clean. Falsifiability by mutation — each piece broken in place, suite re-run, then reverted:

mutation	failure
reindex never erases	nothing stale left under the old key: expected nil, got table
store upsert does not erase the replaced row	bare upsert erased the replaced row's key: expected nil, got table
remove pk path drops its reindex	nothing stale left under the old key: expected nil, got table
remove pk-less path drops its reindex	pk-less value change left no stale key: expected nil, got table
drop_table does not clear the index	drop_table emptied the index with them: expected nil, got table

Reviewer attention

• stdb/schema.lua shows +34/-34 for what is described as adding one has_column helper. Please confirm that is a genuine change and not incidental reformatting.
• #35 limits the motivating win. An Identity-typed pk currently decodes to a table, so a unique identity column has the same problem — which is the exact example this feature exists to serve. Noted in a comment; not fixed here, since #35 changes a frozen row-field type.

Closes #28. `cache.get` looks up by primary key only; any other unique column needed a full `cache.iter` scan. Now that the generator emits `unique`, the cache maintains a lookup per unique column: ```lua cache.find(state, "player", "identity", my_identity) ``` The motivating shape is the common one in a game: a `player` table keyed by auto-increment `id` with a unique `identity` column. "Which row is me?" runs on every connect and every reconnect, and it was O(n) on the main thread. ## Design decisions - **`state.indexes[table][column][value] = row`**, built once from the schema at `cache.new`, rebuilt by `drop_all`. - **The pk column is deliberately not mirrored.** `state.tables[name]` already *is* that index; mirroring would double memory and leave two structures to keep in step. - **`find` on the pk column delegates to `get`**, so callers — and future generated accessors — route every single-column unique lookup through one function without special-casing the pk. `get` stays the direct spelling. Both are frozen at 1.0, so the relationship is spelled out in `find`'s docstring. - **Maintenance lives inside the private `store` / `remove`**, which are only ever reached from `apply_database_update`. No separate pass, and #21's pairing stays purely an event concern — the index needs no second diff. - **`find` on a non-unique column raises**, rather than returning nil or scanning. Returning nil would be indistinguishable from a genuine miss and would hide a lookup that can never succeed; a scan would be a silent O(n) on the main thread. A *unique* column with no match returns nil, so hit / miss / unusable are three distinguishable outcomes. - **`schema.new` now rejects a `unique` name that is not a column**, for the same reason it already rejects such a pk: it would build an index no row can populate, making `find` miss forever instead of saying why. ## The correctness case that matters A unique column's value changing must erase the old key before writing the new one, or the index keeps a stale entry pointing at a row that no longer holds that value. That falls out of the existing delete-then-insert ordering — the delete half carries the old row, so the old key is erased before the insert writes the new one. The case ordering does **not** cover is a row change arriving as a bare insert with no delete half, which the pk store has always treated as an upsert; `store` therefore explicitly erases the keys of the row it replaces. Separately tested. `reindex`'s `old` side is always the row the cache is holding, never a row straight off the wire, so an entry can only ever be erased by the row that owns it. Tables with no `unique` metadata get no index entry and pay one nil check. Hand-written schemas without a `unique` field work unchanged. ## Verification `5 passed, 0 failed`; stylua clean. Falsifiability by mutation — each piece broken in place, suite re-run, then reverted: | mutation | failure | |---|---| | `reindex` never erases | `nothing stale left under the old key: expected nil, got table` | | `store` upsert does not erase the replaced row | `bare upsert erased the replaced row's key: expected nil, got table` | | `remove` pk path drops its `reindex` | `nothing stale left under the old key: expected nil, got table` | | `remove` pk-less path drops its `reindex` | `pk-less value change left no stale key: expected nil, got table` | | `drop_table` does not clear the index | `drop_table emptied the index with them: expected nil, got table` | ## Reviewer attention - `stdb/schema.lua` shows +34/-34 for what is described as adding one `has_column` helper. Please confirm that is a genuine change and not incidental reformatting. - **#35 limits the motivating win.** An `Identity`-typed pk currently decodes to a table, so a unique `identity` column has the same problem — which is the exact example this feature exists to serve. Noted in a comment; not fixed here, since #35 changes a frozen row-field type.

yuris added 1 commit 2026-08-19 17:58:38 +00:00

cache: index rows by every single-column unique constraint (#28) ... caaa9dab7a

"Which row is me?" is the lookup a game makes on every connect and after
every reconnect: a `player` table keyed by an auto-increment `id`, and the
key the client actually holds is the unique `identity` column. `cache.get`
only answers by primary key, so that question cost a full `cache.iter` scan
on the main thread. The generator now emits `unique` per table, so the cache
can maintain a lookup per unique column exactly as it maintains the pk store.

`cache.find(state, table, column, value)` reads those lookups. It accepts the
pk column too and delegates to `cache.get` for it, so a caller can route
every single-column unique lookup through one function without special-casing
the pk; `get` stays the direct spelling and neither shadows the other. Both
are frozen at 1.0, so the relationship is spelled out in find's docstring.

The pk itself is deliberately NOT mirrored into a second table: `state.tables`
already is that index, and a copy would double the memory and leave two
structures to keep in step.

Maintenance rides inside `store` and `remove` rather than in a pass of its
own. That is what makes the case a naive index gets wrong come out right for
free: a changed indexed column arrives as a delete plus an insert, deletes are
applied first, and the delete half carries the old row — so the old key is
erased before the new one is written. #21's delete/insert pairing stays purely
an event concern; the index needs no second diff. The one case ordering does
not cover is a row change sent as a bare insert with no delete half, which the
pk store has always taken as an upsert, so `store` erases the keys of the row
it is replacing.

`reindex` only ever reads the row the cache is holding, never one straight off
the wire, so an entry can only be erased by the row that owns it.

A table with no `unique` metadata allocates nothing and costs store/remove a
single nil check, which keeps hand-written schemas — no `unique` field at all —
working as before. `find` raises for a column under no unique constraint:
returning nil would be indistinguishable from a genuine miss and would hide a
lookup that can never succeed, and scanning instead would be a silent O(n) on
the main thread.

`schema.new` now checks that every name in `unique` is a column, for the same
reason it already checks the pk: a unique column that is not a column builds an
index no row can populate, so `find` on it would miss forever instead of
saying why.

Does not address #35: `identity` still decodes to a table per row, and a table
key matches only itself, so identity-keyed lookups are broken in `get` and in
`find` alike — including the motivating `player.identity` case. The index is
right; the key type is not, yet. Noted in find's docstring.

yuris commented 2026-08-19 18:12:03 +00:00

Author

Owner

Copy link

Review — automated reviewer, posted for the record

Review — PR #44, feat/28-unique-index

Checked out at caaa9da, base 7fd668a. Suite on the branch: 5 passed, 0 failed. All findings below are reproduced by probes I ran against the branch; the proposed fixes are applied to a scratch copy (/tmp/fix-44) and re-verified.

The design is right. state.indexes[table][column][value] = row, not mirroring the pk, maintenance inside store/remove, no second diff of #21's pairing — all of that is the correct shape, and the author's own prose at stdb/cache.lua:69-71 states the correct invariant. The code does not implement it. One line fixes it.

---

BLOCKER — reindex erases index entries it does not own, losing rows on server-legal input

stdb/cache.lua:72-87, specifically the unconditional erase at :76-78:

local prev = old[col]
if prev ~= nil then
	by_value[prev] = nil   -- erases whatever is under `prev`, not necessarily `old`
end

The docstring one line above claims the opposite:

stdb/cache.lua:69-71 — "old is always the row the cache is holding, never one straight off the wire, so an entry can only ever be erased by the row that owns it."

The premise is true; the conclusion does not follow. old is indeed a cached row, but by_value[old[col]] may already have been reassigned to a different row earlier in the same batch. The erase then deletes a live entry, and nothing ever rewrites it. The row stays in state.tables and becomes permanently unreachable through find — until its indexed column changes again or drop_all runs.

Why this is reachable without a server-side constraint violation

apply_database_update loops updates entries at stdb/cache.lua:209. Deletes-first ordering (:223-228) is per entry, not per batch — so across two entries an insert can precede the delete of the row that held its value. Each entry is one subscribed query's diff, so two queries over the same table produce two entries.

That is not hypothetical. The repo's own captured live 2.8.2 session already contains it — dev/test/fixtures/session.jsonl, line 24:

table_name=message  num_rows=4  #updates=2
  entry 1: +[5,"editor","after edit"]  -[5,"editor","before edit"]
  entry 2: +[5,"editor","after edit"]  -[5,"editor","before edit"]

Minimal repro — every update a complete delete+insert pair, no unique value ever duplicated among live rows

local d = schema.new({ acct = { columns = {"id","handle"}, pk = "id", unique = {"id","handle"} } })
local s = cache.new(d)
cache.apply_database_update(s, one("acct", { '[1,"a"]', '[2,"b"]' }, {}))
-- two subscriptions (WHERE id=1, WHERE id=2); a reducer swaps their handles
cache.apply_database_update(s, { tables = { { table_name = "acct", updates = {
  { deletes = { '[1,"a"]' }, inserts = { '[1,"b"]' } },   -- entry 1
  { deletes = { '[2,"b"]' }, inserts = { '[2,"a"]' } },   -- entry 2
} } } })

Observed:

store: id1.handle=b  id2.handle=a      <- store is correct
index: find(a)=2     find(b)=nil       <- row 1 is unreachable, forever

Trace: entry 1 erases ["a"], writes ["b"]=row1. Entry 2 deletes row 2, whose cached handle is still "b", so reindex erases ["b"] — row 1's freshly written entry — then writes ["a"]=row2.

Three more shapes, same root cause

case	result on branch
Two rows swap unique values as bare inserts, one entry (the upsert path the PR explicitly supports and tests for one row)	find("b") == nil, row 1 stored and unreachable
Delete+insert split across entries, insert entry first, value transferred between rows	surviving row unreachable
Insert colliding with an existing unique value, then the displaced owner is deleted	the surviving row's entry is erased

Randomised property check — pk + two unique columns, 300 steps × 40 seeds, asserting after every step that every index entry points at a present row holding that value and every stored row is reachable through each of its unique columns:

• paired updates only (delete+insert always present, server-legal throughout): fails, seed 1 step 39: find(handle=h15) != get(6)
• with bare-insert upserts mixed in: 40/40 seeds fail, first at step 15

Fix — one line, makes the existing docstring true

if prev ~= nil and by_value[prev] == old then
	by_value[prev] = nil
end

Verified on /tmp/fix-44: both property runs pass (0/40 seeds fail), every hand-written case above passes, existing suite still 5 passed, 0 failed. Cost is within run-to-run noise and memory is unchanged (numbers in §6).

The two cases that still violate the invariant afterwards both require the server to have already violated its own unique constraint (an insert colliding with a live value; duplicate unique values in a pk-less table). In both, one row shadowing the other is the only sensible outcome, and with the guard no legal row is ever lost — which is the part that matters.

Test coverage gap that hid this

dev/test/test_cache.lua:24-26:

local function tbl(name, inserts, deletes)
	return { table_name = name, num_rows = #inserts + #deletes, updates = { { inserts = inserts, deletes = deletes } } }
end

One updates entry, hardcoded. Every cache test in the repo goes through this helper, so no test can express more than one entry per table — the exact shape the fixture contains and the bug lives in. Please add a two-entry test alongside the swap case; the helper needs a variant that takes a list of entries.

---

SHOULD-FIX — the level-2 trace does not point at the caller, contradicting find's own docstring

stdb/cache.lua:308 promises "A stack trace at the callsite beats both." It does not deliver one. err (:20-22) calls error(msg, 2) from inside err, so level 2 resolves to the frame that called err — find itself:

called from game/player.lua:2  ->  /tmp/review-44/stdb/cache.lua:324: stdb.cache: table "a" has no unique column "level"

The caller is never named. schema.lua in this same PR gets it right by calling error(..., 2) inline:

called from game/player.lua:2  ->  game/player.lua:2: stdb.schema: unique column "b" is not a column of "t"

This is pre-existing behaviour of err, and a shared err cannot have one correct level — store's use is reached from apply_database_update, where no level points at user code. So raise inline in find:

error('stdb.cache: table "' .. tostring(name) .. '" has no unique column "' .. tostring(column) .. '"', 2)

Verified: game/player.lua:2: stdb.cache: table "a" has no unique column "level". Suite unaffected.

Worth fixing here rather than later: this error is the whole justification for raising instead of returning nil, and 1.0 freezes the contract. An error that misattributes itself to the SDK sends the reader into cache.lua instead of to their typo.

SHOULD-FIX — the README promises the one use case #35 makes non-functional

The PR body and stdb/cache.lua:311-315 are honest about #35. README.md:656-658 is not, and the README is what users read:

"cache.find answers "which row is me?" — the lookup a player table keyed by an auto-increment id needs on every connect, where the key you hold is the unique identity column and not the pk."

Measured against the fixture's real wire encoding ({"__identity__":"0x…"}; protocol.decode_row at stdb/protocol.lua:163-188 never calls M.unwrap, so the field stays a fresh table per row):

lookup	result
find(s, "player", "identity", ME) — the identity string IdentityToken hands you	nil
find(s, "player", "identity", { __identity__ = ME }) — re-wrapped as the wire spells it	nil
find(s, "player", "identity", row.identity) — the table off that same row	works, and is circular
the same, kept across a drop_all + resubscribe	nil (row re-decoded, new table)

So the advertised lookup delivers zero — not "impaired". The index/store invariant is fine; the key type is the problem, exactly as the docstring says. One sentence in the README, next to the promise, closes the gap. A string, integer or u32 unique column (handle, slug, email, an account number) works fully today, which is real value — just not the value the README leads with.

SHOULD-FIX — add the multi-entry test (see the coverage gap above)

Separated out because it stands on its own even after the blocker is fixed: nothing in the suite exercises stdb/cache.lua:209's loop with more than one iteration, for the index or for #21's events.

NIT — two unique validation holes, in a function whose job is diagnostics

stdb/schema.lua:49:

• unique = "handle" (a bare string) → stdb/schema.lua:49: bad argument #1 to 'ipairs' (table expected, got string). Not an stdb.schema: diagnostic, and it names the SDK's line. columns has the same hole pre-existing, so a shared type check would close both.
• unique = { handle = true } (set-shaped — a plausible hand-written spelling, and the shape indexes[].unique uses as a boolean) passes validation, builds no index, and then find(s, "t", "handle", v) raises has no unique column "handle" — a message that flatly contradicts the schema the user wrote. Rejecting a non-empty unique with no [1] would cost two lines and turn a confusing runtime raise into a load-time one.

Both are cheap and this is a freeze.

NIT — state.indexes is now a hard requirement of find, store, remove and drop_table

stdb/cache.lua:96, :123, :263, :321 index state.indexes unguarded, so a state not built by cache.new fails with attempt to index field 'indexes' (a nil value). connection.lua:19 uses cache.new, so nothing internal breaks and hand-constructing state is not a documented pattern. Recording it only because it is a silent shape requirement added to a frozen module.

---

§3 — find delegating to get for the pk column

stdb/cache.lua:316-320. Both checks pass.

Pk-less table: the guard is def.pk ~= nil and column == def.pk, so on a pk-less table no delegation can fire and the unique column resolves through the index. Verified: find(s, "slug", "slug", "x") returns the row; find(s, "slug", "note", …) raises. Correct.

Worth knowing (pre-existing, not this PR): get on a pk-less table indexes the array, so cache.get(s, "slug", 1) returns row 1 by list position. find is therefore the only correct keyed lookup for pk-less tables — which strengthens the case for find existing.

Agreement on a pk column: find(s, "a", "id", v) is identical to get(s, "a", v) across 12 key shapes — 1, 2, 404, 0, -1, "1", "", true, false, 1.0, 1.5, nil. No disagreement. Lua 5.1's all-doubles numbers mean 1.0 and 1 are the same key, so no float/int split to worry about. find(pk) also works when the schema carries no unique field at all.

Judgement: keep the delegation. It earns its keep concretely — the generator emits unique including the pk, so generated accessors can loop one list and emit find_by_<col> for every entry with no pk special case. That is the payoff, and it is why the pk is in unique at all. Against it: two frozen functions where one is a superset invites "which do I use?", and "get stays the direct spelling" is a style rule rather than a semantic one. That is a mild cost, and find's docstring already spells the relationship out. One suggestion: put the reason in the README table too — without "so a generated accessor loop needs no pk special case", the line reads as a redundant API.

§4 — raising for a non-unique column

• Error text: stdb.cache: table "a" has no unique column "level". Names the table and the column. Good.
• Level-2 trace: broken, see the should-fix above.
• Unique column, no match → nil, no raise. Verified.
• Unknown table → raises (has no unique column "id"). nil column → raises. Both reasonable.

Judgement: raising is right, and acceptable to freeze.

For a game, a typo'd column name returning nil is the worse outcome: "this lookup can never succeed" becomes indistinguishable from "this player isn't loaded yet", and it manifests as a player that silently never spawns, with no diagnostic anywhere. Raising is the ordinary programmer-error/data-absence split, and it is the same call schema.new already makes for a bad pk.

On regeneration: yes, a column losing its unique constraint server-side turns previously-working find calls into runtime raises, on a path that runs on every connect. That is a real hazard — but it argues for raising, not against. Silently returning nil there would convert "this constraint no longer exists" into "this player does not exist", which in a game is a much harder failure to diagnose and a much worse one to ship. A crash on the connect path is found in the first playtest; a player that intermittently fails to load is found by users.

Worth a follow-up, not a change here: if the generator emits named find_by_<col> accessors from unique, losing the constraint deletes the accessor and the break surfaces as attempt to call nil at the callsite — earlier and more legible than a raise from inside the cache. schema.new itself cannot pre-check this, since it cannot know which columns callers will find on.

§5 — the schema.lua diff

The +34/-34 in the PR body is a misread. git diff --numstat gives +24/-10; the 34 in --stat is the churn total (24+10), not two symmetric numbers.

It is a genuine change, no incidental reformatting. The 10 removed lines are exactly the inlined pk search (local found = false … if not found then), now has_column. The 24 added are the helper (:16-24), the unique doc block (:16-21 of the header comment), the one-line pk check, and the unique loop with its comment. The only line that moved without changing meaning is the pk error(...), re-indented because collapsing if def.pk ~= nil then removed a nesting level — a consequence of the change, not a reformat.

Breaking change: yes, and it is the right one. unique was previously never read, so a hand-written schema with unique = { "typo" } loaded fine and now raises stdb.schema: unique column "typo" is not a column of "t". Verified. Acceptable: such a schema was already wrong in intent, the failure is loud, it fires at init before any gameplay, the message names the offending column, and it correctly attributes to the caller's file. Same precedent as the existing pk check. The two validation holes in the nit above are the only loose ends.

§6 — cost

Best-of-5, three independent runs, Lua 5.1, 20 000 rows, 4 columns. Same script both sides (unique is simply ignored by main, so the schema input is identical).

case	main	branch	delta
InitialSubscription, 20k rows, no unique columns	0.0841 s	0.0851 s	+1.2% — noise, effectively free
InitialSubscription, 20k rows, two unique columns	0.0830 s	0.0896 s	+8.0% (≈0.33 µs/row for 2 columns)
5 000 transaction updates, no unique columns	0.0510 s	0.0511 s	+0.2% — noise
5 000 transaction updates, two unique columns, both changing	0.0542 s	0.0590 s	+8.9% (≈0.96 µs per updated row)

Memory held by the cache after the same 20k-row load:

case	main	branch	delta
no unique columns	4887 KB	4888 KB	+1 KB — free
two unique columns	4887 KB	7448 KB	+2561 KB, +52%

References, not copies — verified by identity: state.indexes.uniqrow.handle["h7"] == cache.get(state, "uniqrow", 7) is true. The +2561 KB is 40 000 hash entries at ≈64 B, which is a Lua 5.1 Node plus power-of-two sizing slack (32768 slots for 20000 keys), not row duplication.

Framing for #30, no fix demanded: the "no unique columns" claim in the PR body holds — one nil check at stdb/cache.lua:96 and :123, unmeasurable. For an indexed table, a 100-row transaction costs about 0.1 ms extra, ~0.6% of a 60 fps frame. Negligible. The one number worth writing down somewhere is the memory: +52% on the cached footprint of a table with two non-pk unique columns. The PR body discusses "double the memory" only in the context of the pk mirroring it correctly avoided; it does not mention that each non-pk unique column costs a full hash entry per row. Not a defect, just an undocumented cost for anyone caching a large table.

The proposed guard adds nothing measurable: fixed vs branch across two runs stays within ±3% on every case, memory identical.

§7 — compound uniqueness

Clean, and correctly documented. With a table carrying unique = { "a" } (pk only) plus indexes = { { name = "b_c", columns = { "b", "c" }, unique = true } }:

• state.indexes.pair is nil — no index built, no wrong single-column index. build_indexes (stdb/cache.lua:36-49) reads only def.unique and never touches def.indexes; verified no entry leaks in.
• find(s, "pair", "b", "x") raises table "pair" has no unique column "b" — accurate, and the right answer.
• Documented in both places: stdb/schema.lua:17-21 ("Compound uniqueness is not here: one client-side lookup needs one column") and README.md:528 ("Compound uniqueness lives only on indexes[].unique").

No claim of support anywhere, no silent wrong index. Nothing to change. (schema.new does not validate indexes[].columns names at all, but nothing reads them yet — out of scope.)

§8 — does it close #28?

No. Four of five acceptance bullets are met; the third is met only for the shape the test helper can express.

#28 acceptance	status
lookup hit	✓ test_cache.lua "hit: unique column finds its row"
lookup miss	✓
a value change moving the entry and leaving no stale key	✗ — holds for one row in one updates entry; fails for two rows swapping, for bare-insert upserts of two rows, and for any delete/insert split across updates entries. This is the bullet the issue calls "the case a naive version gets wrong"
a delete removing the entry	✓ both pk and pk-less paths
index surviving drop_table / drop_all	✓ including that both leave a usable index behind
#21 interaction: don't diff the pair twice	✓ satisfied better than asked — maintenance rides inside store/remove, so there is no second diff at all

On how much value lands: the mechanism is correct and the string/integer unique-column case is fully working and genuinely useful. The motivating example both the issue and the README lead with — player.identity — delivers nothing today (§ the #35 should-fix), and no amount of index correctness changes that, because the defect is the row-field type.

On honesty: the PR body is honest, prominently, under "Reviewer attention", and stdb/cache.lua:311-315 carries the caveat where a maintainer will read it. That is the right instinct. The gap is that the README carries the promise without the caveat, and the README is the user-facing document. One sentence.

---

To merge

1. The reindex identity guard, plus a test with two updates entries and a two-row swap. This is the blocker; everything else is small.
2. Raise inline at level 2 in find, so the error delivers the callsite its own docstring promises.
3. One sentence in the README.md cache.find blurb noting the identity case is blocked on #35.
4. Optional, cheap, and a freeze: reject a non-array unique; correct the PR body's +34/-34 to +24/-10 (it is a genuine change).

The author asked two questions. schema.lua is a real change, +24/-10, no reformatting. And #35 does not merely limit the motivating win — it eliminates it, so the README should say so where users will see it.

## Review — automated reviewer, posted for the record # Review — PR #44, `feat/28-unique-index` Checked out at `caaa9da`, base `7fd668a`. Suite on the branch: `5 passed, 0 failed`. All findings below are reproduced by probes I ran against the branch; the proposed fixes are applied to a scratch copy (`/tmp/fix-44`) and re-verified. The design is right. `state.indexes[table][column][value] = row`, not mirroring the pk, maintenance inside `store`/`remove`, no second diff of #21's pairing — all of that is the correct shape, and the author's own prose at `stdb/cache.lua:69-71` states the correct invariant. The code does not implement it. One line fixes it. --- ## BLOCKER — `reindex` erases index entries it does not own, losing rows on server-legal input `stdb/cache.lua:72-87`, specifically the unconditional erase at `:76-78`: ```lua local prev = old[col] if prev ~= nil then by_value[prev] = nil -- erases whatever is under `prev`, not necessarily `old` end ``` The docstring one line above claims the opposite: > `stdb/cache.lua:69-71` — "`old` is always the row the cache is holding, never one straight off the wire, so an entry can only ever be erased by the row that owns it." The premise is true; the conclusion does not follow. `old` is indeed a cached row, but `by_value[old[col]]` may already have been reassigned to a **different** row earlier in the same batch. The erase then deletes a live entry, and nothing ever rewrites it. The row stays in `state.tables` and becomes permanently unreachable through `find` — until its indexed column changes again or `drop_all` runs. ### Why this is reachable without a server-side constraint violation `apply_database_update` loops `updates` entries at `stdb/cache.lua:209`. Deletes-first ordering (`:223-228`) is **per entry**, not per batch — so across two entries an insert can precede the delete of the row that held its value. Each entry is one subscribed query's diff, so two queries over the same table produce two entries. That is not hypothetical. The repo's own captured live 2.8.2 session already contains it — `dev/test/fixtures/session.jsonl`, line 24: ``` table_name=message num_rows=4 #updates=2 entry 1: +[5,"editor","after edit"] -[5,"editor","before edit"] entry 2: +[5,"editor","after edit"] -[5,"editor","before edit"] ``` ### Minimal repro — every update a complete delete+insert pair, no unique value ever duplicated among live rows ```lua local d = schema.new({ acct = { columns = {"id","handle"}, pk = "id", unique = {"id","handle"} } }) local s = cache.new(d) cache.apply_database_update(s, one("acct", { '[1,"a"]', '[2,"b"]' }, {})) -- two subscriptions (WHERE id=1, WHERE id=2); a reducer swaps their handles cache.apply_database_update(s, { tables = { { table_name = "acct", updates = { { deletes = { '[1,"a"]' }, inserts = { '[1,"b"]' } }, -- entry 1 { deletes = { '[2,"b"]' }, inserts = { '[2,"a"]' } }, -- entry 2 } } } }) ``` Observed: ``` store: id1.handle=b id2.handle=a <- store is correct index: find(a)=2 find(b)=nil <- row 1 is unreachable, forever ``` Trace: entry 1 erases `["a"]`, writes `["b"]=row1`. Entry 2 deletes row 2, whose cached `handle` is still `"b"`, so `reindex` erases `["b"]` — **row 1's freshly written entry** — then writes `["a"]=row2`. ### Three more shapes, same root cause | case | result on branch | |---|---| | Two rows swap unique values as **bare inserts**, one entry (the upsert path the PR explicitly supports and tests for one row) | `find("b") == nil`, row 1 stored and unreachable | | Delete+insert split across entries, insert entry first, value transferred between rows | surviving row unreachable | | Insert colliding with an existing unique value, then the displaced owner is deleted | the **surviving** row's entry is erased | Randomised property check — pk + two unique columns, 300 steps × 40 seeds, asserting after every step that every index entry points at a present row holding that value and every stored row is reachable through each of its unique columns: - **paired updates only** (delete+insert always present, server-legal throughout): fails, `seed 1 step 39: find(handle=h15) != get(6)` - with bare-insert upserts mixed in: **40/40 seeds fail**, first at step 15 ### Fix — one line, makes the existing docstring true ```lua if prev ~= nil and by_value[prev] == old then by_value[prev] = nil end ``` Verified on `/tmp/fix-44`: both property runs pass (`0/40 seeds fail`), every hand-written case above passes, existing suite still `5 passed, 0 failed`. Cost is within run-to-run noise and memory is unchanged (numbers in §6). The two cases that still violate the invariant afterwards both require the server to have already violated its own unique constraint (an insert colliding with a live value; duplicate unique values in a pk-less table). In both, one row shadowing the other is the only sensible outcome, and with the guard **no legal row is ever lost** — which is the part that matters. ### Test coverage gap that hid this `dev/test/test_cache.lua:24-26`: ```lua local function tbl(name, inserts, deletes) return { table_name = name, num_rows = #inserts + #deletes, updates = { { inserts = inserts, deletes = deletes } } } end ``` One `updates` entry, hardcoded. Every cache test in the repo goes through this helper, so **no test can express more than one entry per table** — the exact shape the fixture contains and the bug lives in. Please add a two-entry test alongside the swap case; the helper needs a variant that takes a list of entries. --- ## SHOULD-FIX — the level-2 trace does not point at the caller, contradicting `find`'s own docstring `stdb/cache.lua:308` promises "A stack trace at the callsite beats both." It does not deliver one. `err` (`:20-22`) calls `error(msg, 2)` from **inside `err`**, so level 2 resolves to the frame that called `err` — `find` itself: ``` called from game/player.lua:2 -> /tmp/review-44/stdb/cache.lua:324: stdb.cache: table "a" has no unique column "level" ``` The caller is never named. `schema.lua` in this same PR gets it right by calling `error(..., 2)` inline: ``` called from game/player.lua:2 -> game/player.lua:2: stdb.schema: unique column "b" is not a column of "t" ``` This is pre-existing behaviour of `err`, and a shared `err` cannot have one correct level — `store`'s use is reached from `apply_database_update`, where no level points at user code. So raise inline in `find`: ```lua error('stdb.cache: table "' .. tostring(name) .. '" has no unique column "' .. tostring(column) .. '"', 2) ``` Verified: `game/player.lua:2: stdb.cache: table "a" has no unique column "level"`. Suite unaffected. Worth fixing here rather than later: this error is the whole justification for raising instead of returning nil, and 1.0 freezes the contract. An error that misattributes itself to the SDK sends the reader into `cache.lua` instead of to their typo. ## SHOULD-FIX — the README promises the one use case #35 makes non-functional The PR body and `stdb/cache.lua:311-315` are honest about #35. `README.md:656-658` is not, and the README is what users read: > "`cache.find` answers \"which row is *me*?\" — the lookup a `player` table keyed by an auto-increment `id` needs on every connect, where the key you hold is the unique `identity` column and not the pk." Measured against the fixture's real wire encoding (`{"__identity__":"0x…"}`; `protocol.decode_row` at `stdb/protocol.lua:163-188` never calls `M.unwrap`, so the field stays a fresh table per row): | lookup | result | |---|---| | `find(s, "player", "identity", ME)` — the identity string `IdentityToken` hands you | **nil** | | `find(s, "player", "identity", { __identity__ = ME })` — re-wrapped as the wire spells it | **nil** | | `find(s, "player", "identity", row.identity)` — the table off that same row | works, and is circular | | the same, kept across a `drop_all` + resubscribe | **nil** (row re-decoded, new table) | So the advertised lookup delivers **zero** — not "impaired". The index/store invariant is fine; the key type is the problem, exactly as the docstring says. One sentence in the README, next to the promise, closes the gap. A string, integer or `u32` unique column (`handle`, `slug`, `email`, an account number) works fully today, which is real value — just not the value the README leads with. ## SHOULD-FIX — add the multi-entry test (see the coverage gap above) Separated out because it stands on its own even after the blocker is fixed: nothing in the suite exercises `stdb/cache.lua:209`'s loop with more than one iteration, for the index or for #21's events. ## NIT — two `unique` validation holes, in a function whose job is diagnostics `stdb/schema.lua:49`: - `unique = "handle"` (a bare string) → `stdb/schema.lua:49: bad argument #1 to 'ipairs' (table expected, got string)`. Not an `stdb.schema:` diagnostic, and it names the SDK's line. `columns` has the same hole pre-existing, so a shared type check would close both. - `unique = { handle = true }` (set-shaped — a plausible hand-written spelling, and the shape `indexes[].unique` uses as a boolean) **passes validation**, builds no index, and then `find(s, "t", "handle", v)` raises `has no unique column "handle"` — a message that flatly contradicts the schema the user wrote. Rejecting a non-empty `unique` with no `[1]` would cost two lines and turn a confusing runtime raise into a load-time one. Both are cheap and this is a freeze. ## NIT — `state.indexes` is now a hard requirement of `find`, `store`, `remove` and `drop_table` `stdb/cache.lua:96`, `:123`, `:263`, `:321` index `state.indexes` unguarded, so a state not built by `cache.new` fails with `attempt to index field 'indexes' (a nil value)`. `connection.lua:19` uses `cache.new`, so nothing internal breaks and hand-constructing state is not a documented pattern. Recording it only because it is a silent shape requirement added to a frozen module. --- ## §3 — `find` delegating to `get` for the pk column `stdb/cache.lua:316-320`. Both checks pass. **Pk-less table:** the guard is `def.pk ~= nil and column == def.pk`, so on a pk-less table no delegation can fire and the unique column resolves through the index. Verified: `find(s, "slug", "slug", "x")` returns the row; `find(s, "slug", "note", …)` raises. Correct. Worth knowing (pre-existing, not this PR): `get` on a pk-less table indexes the array, so `cache.get(s, "slug", 1)` returns row 1 by list position. `find` is therefore the *only* correct keyed lookup for pk-less tables — which strengthens the case for `find` existing. **Agreement on a pk column:** `find(s, "a", "id", v)` is identical to `get(s, "a", v)` across 12 key shapes — `1`, `2`, `404`, `0`, `-1`, `"1"`, `""`, `true`, `false`, `1.0`, `1.5`, `nil`. No disagreement. Lua 5.1's all-doubles numbers mean `1.0` and `1` are the same key, so no float/int split to worry about. `find(pk)` also works when the schema carries no `unique` field at all. **Judgement: keep the delegation.** It earns its keep concretely — the generator emits `unique` *including the pk*, so generated accessors can loop one list and emit `find_by_<col>` for every entry with no pk special case. That is the payoff, and it is why the pk is in `unique` at all. Against it: two frozen functions where one is a superset invites "which do I use?", and "`get` stays the direct spelling" is a style rule rather than a semantic one. That is a mild cost, and `find`'s docstring already spells the relationship out. One suggestion: put the *reason* in the README table too — without "so a generated accessor loop needs no pk special case", the line reads as a redundant API. ## §4 — raising for a non-unique column - Error text: `stdb.cache: table "a" has no unique column "level"`. Names the table and the column. Good. - Level-2 trace: **broken**, see the should-fix above. - Unique column, no match → `nil`, no raise. Verified. - Unknown table → raises (`has no unique column "id"`). `nil` column → raises. Both reasonable. **Judgement: raising is right, and acceptable to freeze.** For a game, a typo'd column name returning nil is the worse outcome: "this lookup can never succeed" becomes indistinguishable from "this player isn't loaded yet", and it manifests as a player that silently never spawns, with no diagnostic anywhere. Raising is the ordinary programmer-error/data-absence split, and it is the same call `schema.new` already makes for a bad pk. On regeneration: yes, a column losing its unique constraint server-side turns previously-working `find` calls into runtime raises, on a path that runs on every connect. That is a real hazard — but it argues **for** raising, not against. Silently returning nil there would convert "this constraint no longer exists" into "this player does not exist", which in a game is a much harder failure to diagnose and a much worse one to ship. A crash on the connect path is found in the first playtest; a player that intermittently fails to load is found by users. Worth a follow-up, not a change here: if the generator emits named `find_by_<col>` accessors from `unique`, losing the constraint deletes the accessor and the break surfaces as `attempt to call nil` at the callsite — earlier and more legible than a raise from inside the cache. `schema.new` itself cannot pre-check this, since it cannot know which columns callers will `find` on. ## §5 — the `schema.lua` diff **The +34/-34 in the PR body is a misread.** `git diff --numstat` gives **+24/-10**; the `34` in `--stat` is the churn total (24+10), not two symmetric numbers. It is a genuine change, no incidental reformatting. The 10 removed lines are exactly the inlined pk search (`local found = false` … `if not found then`), now `has_column`. The 24 added are the helper (`:16-24`), the `unique` doc block (`:16-21` of the header comment), the one-line pk check, and the `unique` loop with its comment. The only line that moved without changing meaning is the pk `error(...)`, re-indented because collapsing `if def.pk ~= nil then` removed a nesting level — a consequence of the change, not a reformat. **Breaking change: yes, and it is the right one.** `unique` was previously never read, so a hand-written schema with `unique = { "typo" }` loaded fine and now raises `stdb.schema: unique column "typo" is not a column of "t"`. Verified. Acceptable: such a schema was already wrong in intent, the failure is loud, it fires at init before any gameplay, the message names the offending column, and it correctly attributes to the caller's file. Same precedent as the existing pk check. The two validation holes in the nit above are the only loose ends. ## §6 — cost Best-of-5, three independent runs, Lua 5.1, 20 000 rows, 4 columns. Same script both sides (`unique` is simply ignored by `main`, so the schema input is identical). | case | main | branch | delta | |---|---|---|---| | `InitialSubscription`, 20k rows, **no** unique columns | 0.0841 s | 0.0851 s | +1.2% — noise, effectively free | | `InitialSubscription`, 20k rows, **two** unique columns | 0.0830 s | 0.0896 s | **+8.0%** (≈0.33 µs/row for 2 columns) | | 5 000 transaction updates, no unique columns | 0.0510 s | 0.0511 s | +0.2% — noise | | 5 000 transaction updates, two unique columns, **both changing** | 0.0542 s | 0.0590 s | **+8.9%** (≈0.96 µs per updated row) | Memory held by the cache after the same 20k-row load: | case | main | branch | delta | |---|---|---|---| | no unique columns | 4887 KB | 4888 KB | +1 KB — free | | two unique columns | 4887 KB | 7448 KB | **+2561 KB, +52%** | **References, not copies** — verified by identity: `state.indexes.uniqrow.handle["h7"] == cache.get(state, "uniqrow", 7)` is `true`. The +2561 KB is 40 000 hash entries at ≈64 B, which is a Lua 5.1 `Node` plus power-of-two sizing slack (32768 slots for 20000 keys), not row duplication. Framing for #30, no fix demanded: the "no unique columns" claim in the PR body holds — one nil check at `stdb/cache.lua:96` and `:123`, unmeasurable. For an indexed table, a 100-row transaction costs about 0.1 ms extra, ~0.6% of a 60 fps frame. Negligible. The one number worth writing down somewhere is the memory: **+52% on the cached footprint of a table with two non-pk unique columns.** The PR body discusses "double the memory" only in the context of the pk mirroring it correctly avoided; it does not mention that each non-pk unique column costs a full hash entry per row. Not a defect, just an undocumented cost for anyone caching a large table. The proposed guard adds nothing measurable: `fixed` vs `branch` across two runs stays within ±3% on every case, memory identical. ## §7 — compound uniqueness Clean, and correctly documented. With a table carrying `unique = { "a" }` (pk only) plus `indexes = { { name = "b_c", columns = { "b", "c" }, unique = true } }`: - `state.indexes.pair` is `nil` — no index built, no wrong single-column index. `build_indexes` (`stdb/cache.lua:36-49`) reads only `def.unique` and never touches `def.indexes`; verified no entry leaks in. - `find(s, "pair", "b", "x")` raises `table "pair" has no unique column "b"` — accurate, and the right answer. - Documented in both places: `stdb/schema.lua:17-21` ("Compound uniqueness is not here: one client-side lookup needs one column") and `README.md:528` ("Compound uniqueness lives **only** on `indexes[].unique`"). No claim of support anywhere, no silent wrong index. Nothing to change. (`schema.new` does not validate `indexes[].columns` names at all, but nothing reads them yet — out of scope.) ## §8 — does it close #28? **No.** Four of five acceptance bullets are met; the third is met only for the shape the test helper can express. | #28 acceptance | status | |---|---| | lookup hit | ✓ `test_cache.lua` "hit: unique column finds its row" | | lookup miss | ✓ | | a value change moving the entry and **leaving no stale key** | ✗ — holds for one row in one `updates` entry; fails for two rows swapping, for bare-insert upserts of two rows, and for any delete/insert split across `updates` entries. This is the bullet the issue calls "the case a naive version gets wrong" | | a delete removing the entry | ✓ both pk and pk-less paths | | index surviving `drop_table` / `drop_all` | ✓ including that both leave a usable index behind | | #21 interaction: don't diff the pair twice | ✓ satisfied better than asked — maintenance rides inside `store`/`remove`, so there is no second diff at all | On how much value lands: the mechanism is correct and the string/integer unique-column case is fully working and genuinely useful. The motivating example both the issue and the README lead with — `player.identity` — delivers nothing today (§ the #35 should-fix), and no amount of index correctness changes that, because the defect is the row-field type. On honesty: the PR body is honest, prominently, under "Reviewer attention", and `stdb/cache.lua:311-315` carries the caveat where a maintainer will read it. That is the right instinct. The gap is that the README carries the promise without the caveat, and the README is the user-facing document. One sentence. --- ## To merge 1. The `reindex` identity guard, plus a test with two `updates` entries and a two-row swap. This is the blocker; everything else is small. 2. Raise inline at level 2 in `find`, so the error delivers the callsite its own docstring promises. 3. One sentence in the `README.md` `cache.find` blurb noting the `identity` case is blocked on #35. 4. Optional, cheap, and a freeze: reject a non-array `unique`; correct the PR body's `+34/-34` to `+24/-10` (it is a genuine change). The author asked two questions. `schema.lua` is a real change, `+24/-10`, no reformatting. And #35 does not merely limit the motivating win — it eliminates it, so the README should say so where users will see it.

yuris referenced this pull request 2026-08-19 18:20:32 +00:00

tables: a typed accessor per table #45

yuris referenced this pull request 2026-08-19 18:20:44 +00:00

protocol: decode every AlgebraicType from the schema tags #46

yuris referenced this pull request 2026-08-19 18:26:34 +00:00

v1.0.0 release checklist #24

yuris force-pushed feat/28-unique-index from caaa9dab7a to 319f835834 2026-08-19 18:29:01 +00:00 Compare

yuris merged commit 6e37098bd4 into main 2026-08-19 18:31:58 +00:00

yuris referenced this pull request from a commit 2026-08-19 18:31:58 +00:00

cache: unique-column lookups maintained from the schema (#44)

yuris referenced this pull request 2026-08-20 01:12:42 +00:00

tables: a typed accessor per table #45

yuris referenced this pull request 2026-08-20 01:20:24 +00:00

protocol: decode every AlgebraicType from the schema tags #46

yuris referenced this pull request from a commit 2026-08-20 01:26:28 +00:00

tables: a typed accessor per table under db.tables (#27)

yuris referenced this pull request from a commit 2026-08-20 01:27:25 +00:00

tables: a typed accessor per table under db.tables (#27)

yuris referenced this pull request 2026-08-20 01:45:51 +00:00

v1.0.0 release checklist #24

yuris referenced this pull request from a commit 2026-08-20 02:15:04 +00:00

Route cached rows through the typed decoder so an Identity key works

yuris referenced this pull request from a commit 2026-08-20 03:16:31 +00:00

Route cached rows through the typed decoder so an Identity key works

yuris referenced this pull request 2026-08-20 04:12:56 +00:00

cache: skip a relation the schema cannot describe, and emit views #49

yuris referenced this pull request 2026-08-20 05:23:04 +00:00

cache: count how many queries hold a row, so an overlapping one survives #51

yuris referenced this pull request 2026-08-20 06:21:50 +00:00

cache: count how many queries hold a row, so an overlapping one survives #51

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

area:defold

Defold engine integration

  

area:docs

Documentation and examples

  

area:infra

Flake, CI, secretspec

  

area:protocol

Wire protocol conformance

  

area:testing

Test harness and fixtures

  

kind:bug

Something broken

  

kind:chore

Maintenance

  

phase:breadth

UnsubscribeMulti, procedures, BSATN

  

phase:core

Core SDK layers (protocol/cache/connection)

  

phase:hardening

Reconnect, token persistence, errors

  

phase:slice

Vertical slice: first playable end-to-end

No labels

area:defold

area:docs

area:infra

area:protocol

area:testing

kind:bug

kind:chore

phase:breadth

phase:core

phase:hardening

phase:slice

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

yuris/spacetimedb-defold!44

No description provided.