yuris/spacetimedb-defold

Fork 0

tables: a typed accessor per table #45

Merged

yuris merged 1 commit from feat/27-table-api into main

2026-08-20 01:29:17 +00:00

yuris commented

2026-08-19 18:20:32 +00:00

Owner

Closes #27.

Row changes arrived as one firehose: every consumer wrote the same loop over evt.counts.events, filtering by table name and branching on event type. Every table in the schema now has an accessor, namespaced under db.tables:

db.tables.message:on_insert(function(row) end)
db.tables.message:on_update(function(old, new) end)   -- primary-key tables only
db.tables.message:on_delete(function(row) end)

db.tables.message:count()
db.tables.message:iter()
db.tables.message:find("id", 7)
db.tables["my-table"]                                 -- for a name that is not a Lua identifier

It is also where the generator's output stops being merely emitted: t.def carries columns, types, pk, unique and indexes, find reads the unique constraints, and on_update exists because of the pk.

Design decisions

A new module, stdb/tables.lua. connection.lua is 750 lines of request correlation, handles and message dispatch, and a facade shares none of it: what it needs is the schema and the cache, both passed in, so tables.lua is pure and unit-testable without an engine. connection.lua gains one require, one install call and a four-line emit_rows.
on_event is untouched and still fires first. The per-table callbacks run after it for every frame, so nothing an existing consumer already saw can be reordered by a callback registered behind it.
Deregistration is spelled the way Subscription:off_applied already spells it: off_insert(fn), removing one registration, the first match. A game object registers in init(self) and the closure holds self; if final(self) cannot undo that, the first row after the object is deleted calls into a corpse. A handle returned by on_insert would have been a second idiom for one concept, a second object for that game object to store, and it would have cost on_insert its return value — which chains instead.
Callbacks fire per row once the whole batch is in the cache, so a callback reading iter/count/find sees a complete transaction, never half of one.
Each row's callback list is copied before any of it runs, for the reason connection.lua's fire copies its own: registering or detaching from inside a callback is ordinary — the fire-once idiom is exactly that — and mutating the list under the loop would either skip a sibling or index past the end. Attaching and detaching therefore take effect from the next row. One buffer refilled per row rather than one table per row, so a 500-row transaction does not allocate 500 of them.
on_update raises on a pk-less table. Such a table can never report an update — its row change arrives as a delete plus an insert, and cache.lua deliberately does not pair those — so the registration could never fire. require_pk reads the same def.pk that cache.lua pairs updates off, so the raise cannot disagree with what dispatch can actually deliver: it is derived, not heuristic. off_update raises too, so a final() mirroring its init() fails in the same place.
find delegates entirely to cache.find (#44). No index maintenance is duplicated here. Passing the pk is allowed and delegates to cache.get, so generated code can loop the one unique list with no pk special case; a column under no unique constraint raises rather than scanning.

Why `db.tables.<name>` and not a flat `db.<name>`

Table names belong to whoever wrote the server module, and the connection object already owns state, send, close, subscribe, identity, and whatever it grows next. A flat accessor merges those two namespaces, and the clash is reachable rather than theoretical: 2.8.2 accepts a table called state or close, publishes it, and serves its rows through SELECT * FROM state (verified against a live server on a scratch module).

Every flat answer is worse:

Let the connection's field win and skip that table's accessor, and the consumer discovers it in a game object's init() as attempt to call method 'on_insert' (a nil value), with db.state handing back the cache.
Raise at connect, and the SDK refuses a legal module outright — unfixable from the consumer's side without renaming a server-side table that may not be theirs.
Either way a reserved list of the connection's own field names becomes public API. Under a 1.0 freeze, adding a field to connection.lua would then be a breaking change for anyone whose module has a table by that name — and adding a name to the list would itself be the breaking change. That is decisive; no list survives it.

Under a namespace the construction is total. A table genuinely called tables is db.tables.tables, nothing can collide, there is no list and nothing is refused; the one reserved name is db.tables itself, which is ours to spend. It also matches the peer SDKs — Godot's client.db.<table> and the official ctx.db.<table> both namespace, and their client/ctx is our db.

Index with brackets for a name that is not a valid Lua identifier: db.tables["my-table"]. Dot access does not reach those — db.tables.2fast is a syntax error, and db.tables.my-table compiles, as db.tables.my - table, then fails at runtime with an arithmetic error. The README says so in the table-accessor section.

Since the last review

Rebased onto 6e37098, and the #44 seam is deleted. Table:find is now return cache.find(...) and nothing else. This matters more than a tidy-up: pre-rebase, find fell back to a pk-only answer, so on the real generated schema profile — no pk, unique slug — resolved nothing, profile:find("slug", …) raised, and exactly one find call in the whole schema worked. That was the README:711 mismatch. Measured on the fixture schema, before and after:

call	before	after
`profile:find("slug", "hero")`	raises "needs #44"	hit
`profile:find("slug", "nope")`	raises "needs #44"	nil
`profile:find("realm", "eu")`	raises "needs #44"	raises `stdb.cache: table "profile" has no unique column "realm"`
`message:find("id", 1)`	hit	hit

A find test on a non-pk unique column. Every existing find test used message.id, where pk and unique coincide, so a pk-only find passed them all. log now declares unique = { "line" } — no pk, one unique column, the shape the generator really produces for profile — and the suite asserts both a hit and a nil miss on it.
db.tables in the module's own docstrings. The re-cut renamed every callsite but missed tables.lua's docstrings, which are the API reference: the canonical Defold init/final block, the iter example and the find example all still showed the flat form, and each raised attempt to index field '<name>' (a nil value) if copied — the exact outcome the module header cites as the reason for the namespace. Also one stale comment in smoke.script.
The find example no longer promises a lookup that cannot work. It was db.player:find("identity", me), which is precisely the unique-identity lookup #44's README documents as broken by #35 — a special-type column decodes to a fresh wrapper table per row, so neither the identity string nor a re-wrapped copy matches, and it returns a silent nil. The example is now find("slug", …), and the docstring carries the #35 caveat explicitly.

err()'s level was wrong on every path that reaches it. It hardcoded level 3, documented as "blames the line that called the facade method". return attach(self, …) is a tail call, and Lua 5.1 discards the frame level 3 counts on, so on_insert/on_delete/on_update raised with no file:line at all — the likeliest real mistake, a typo'd self.on_row that is nil, pointed nowhere — while the pk guard blamed tables.lua itself. Now level 4, correct for all five, since deleting the seam left err reachable only from a helper (attach, require_pk) that a facade method called. (off_insert/off_delete take any argument and raise nothing; detach scans for a match and a final() undoing a registration it never made is not an error.)

	level 3 (shipped)	level 4 (now)
`on_insert(nil)`	(no location at all)	the caller's line
`on_delete(nil)`	(no location at all)	the caller's line
`on_update(nil)`	(no location at all)	the caller's line
`log:on_update(fn)`	`stdb/tables.lua:163`	the caller's line
`log:off_update(fn)`	`stdb/tables.lua:171`	the caller's line

Three dispatch contracts that were real but undocumented, each measured before being written down:
- A callback that throws skips everything behind it — the rest of that table's rows and every later table of the same batch — while the cache is already complete and on_event already ran with the whole batch. A per-table consumer can therefore disagree with an on_event consumer about one transaction, and nothing records which rows were dropped.
- db:close() from inside a callback does not stop the batch: dispatch is a plain loop over already-applied events, so the remaining rows are still delivered.
- "Takes effect from the next row" means the next row of the batch, not of that table. Register on log from a message callback and it receives log's rows of that same transaction — rows cached before it existed.
The bracket form is in the README, and require_pk now records the one residual it has: the guard is only as true as the generated schema, and a stale one inverts it into the silence it exists to prevent. That is #40's "regenerate after any module change" with a sharper consequence, inherited rather than introduced here.

Verification

6 passed, 0 failed; stylua clean; native conformance PASS first try, both runs, on the rebased tree, with the API exercised end to end:

SMOKE: PASS procedure round trip; failure surfaced as an error; per-table API: 1 insert(s), 1 update, 1 delete(s)
SMOKE: PASS procedure round trip; failure surfaced as an error; per-table API: 2 insert(s), 1 update, 2 delete(s)
[conformance] PASS (identity 0xc200c808... persisted across a process restart)

The smoke client registers all three callbacks, calls find by pk inside the update callback and asserts it returns the very row the callback was handed, then off_inserts from inside that callback — phase 3 proves the detach took by counting the rows that the reconnect replay and the phase-3 query set would otherwise have delivered. Regenerating chat_schema.lua off the live server reproduced the committed fixture byte for byte apart from the port in its header comment, so the fixture is current.

Falsifiability by mutation — each piece broken in place, suite re-run, then reverted:

mutation	failure
`emit_rows` no longer dispatches	`one call per row, in order, from a subscription: expected insert:1,insert:2, got`
dispatch iterates the live list, not the copy	`stdb/tables.lua:266: attempt to call field '?' (a nil value)`
`detach` stops removing	`a detached callback stops being delivered to: expected 1, got 2`
`require_pk` made a no-op	`on_update on a pk-less table: expected error, call succeeded`
callbacks dispatched before `on_event`	`on_event runs before the per-table callbacks: expected event,table, got table,event`
`find` answers only the pk (the deleted seam, restored)	`stdb.tables: find("line") needs #44`
`install` skips a name the connection already owns	`test_tables.lua:333: attempt to index field '?' (a nil value)`

The last two are the new coverage: the sixth is the regression the seam deletion closes, and the seventh guards the namespace's totality, which replaced the reserved list.

Reviewer attention

find is fully index-backed now, with one inherited limit. A unique identity column still does not resolve — cache.find keys on the decoded value, and a special type decodes to a fresh table per row (#35). Documented in the docstring and the README row, not worked around here.
An error raised inside cache loses its file:line through the facade. cache.find/iter/count raise at level 2, which lands on the tail call in Table:find and prints no prefix. The message still names the table and the column, so it is self-locating by content; fixing it properly means a level argument on the cache API, which is #44's surface and already merged. Flagging, not fixing.
Nothing new is emitted through on_event. A reconnect still empties the cache with no delete callbacks; that is stated in the module header and the README rather than worked around, since Subscription:on_applied re-firing is the existing answer.

Closes #27. Row changes arrived as one firehose: every consumer wrote the same loop over `evt.counts.events`, filtering by table name and branching on event type. Every table in the schema now has an accessor, namespaced under `db.tables`: ```lua db.tables.message:on_insert(function(row) end) db.tables.message:on_update(function(old, new) end) -- primary-key tables only db.tables.message:on_delete(function(row) end) db.tables.message:count() db.tables.message:iter() db.tables.message:find("id", 7) db.tables["my-table"] -- for a name that is not a Lua identifier ``` It is also where the generator's output stops being merely emitted: `t.def` carries `columns`, `types`, `pk`, `unique` and `indexes`, `find` reads the unique constraints, and `on_update` exists because of the pk. ## Design decisions - **A new module, `stdb/tables.lua`.** connection.lua is 750 lines of request correlation, handles and message dispatch, and a facade shares none of it: what it needs is the schema and the cache, both passed in, so tables.lua is pure and unit-testable without an engine. connection.lua gains one require, one `install` call and a four-line `emit_rows`. - **`on_event` is untouched and still fires first.** The per-table callbacks run after it for every frame, so nothing an existing consumer already saw can be reordered by a callback registered behind it. - **Deregistration is spelled the way `Subscription:off_applied` already spells it:** `off_insert(fn)`, removing one registration, the first match. A game object registers in `init(self)` and the closure holds `self`; if `final(self)` cannot undo that, the first row after the object is deleted calls into a corpse. A handle returned by `on_insert` would have been a second idiom for one concept, a second object for that game object to store, and it would have cost `on_insert` its return value — which chains instead. - **Callbacks fire per row once the whole batch is in the cache**, so a callback reading `iter`/`count`/`find` sees a complete transaction, never half of one. - **Each row's callback list is copied before any of it runs**, for the reason connection.lua's `fire` copies its own: registering or detaching from inside a callback is ordinary — the fire-once idiom is exactly that — and mutating the list under the loop would either skip a sibling or index past the end. Attaching and detaching therefore take effect from the next row. One buffer refilled per row rather than one table per row, so a 500-row transaction does not allocate 500 of them. - **`on_update` raises on a pk-less table.** Such a table can never report an update — its row change arrives as a delete plus an insert, and cache.lua deliberately does not pair those — so the registration could never fire. `require_pk` reads the same `def.pk` that cache.lua pairs updates off, so the raise cannot disagree with what dispatch can actually deliver: it is derived, not heuristic. `off_update` raises too, so a `final()` mirroring its `init()` fails in the same place. - **`find` delegates entirely to `cache.find` (#44).** No index maintenance is duplicated here. Passing the pk is allowed and delegates to `cache.get`, so generated code can loop the one `unique` list with no pk special case; a column under no unique constraint raises rather than scanning. ## Why `db.tables.<name>` and not a flat `db.<name>` Table names belong to whoever wrote the server module, and the connection object already owns `state`, `send`, `close`, `subscribe`, `identity`, and whatever it grows next. A flat accessor merges those two namespaces, and the clash is reachable rather than theoretical: 2.8.2 accepts a table called `state` or `close`, publishes it, and serves its rows through `SELECT * FROM state` (verified against a live server on a scratch module). Every flat answer is worse: - Let the connection's field win and skip that table's accessor, and the consumer discovers it in a game object's `init()` as `attempt to call method 'on_insert' (a nil value)`, with `db.state` handing back the cache. - Raise at connect, and the SDK refuses a legal module outright — unfixable from the consumer's side without renaming a server-side table that may not be theirs. - **Either way a reserved list of the connection's own field names becomes public API.** Under a 1.0 freeze, adding a field to connection.lua would then be a breaking change for anyone whose module has a table by that name — and *adding a name to the list would itself be the breaking change*. That is decisive; no list survives it. Under a namespace the construction is total. A table genuinely called `tables` is `db.tables.tables`, nothing can collide, there is no list and nothing is refused; the one reserved name is `db.tables` itself, which is ours to spend. It also matches the peer SDKs — Godot's `client.db.<table>` and the official `ctx.db.<table>` both namespace, and their `client`/`ctx` is our `db`. Index with brackets for a name that is not a valid Lua identifier: `db.tables["my-table"]`. Dot access does not reach those — `db.tables.2fast` is a syntax error, and `db.tables.my-table` *compiles*, as `db.tables.my - table`, then fails at runtime with an arithmetic error. The README says so in the table-accessor section. ## Since the last review - **Rebased onto `6e37098`, and the #44 seam is deleted.** `Table:find` is now `return cache.find(...)` and nothing else. This matters more than a tidy-up: pre-rebase, `find` fell back to a pk-only answer, so on the real generated schema `profile` — no pk, unique `slug` — resolved *nothing*, `profile:find("slug", …)` raised, and exactly one `find` call in the whole schema worked. That was the README:711 mismatch. Measured on the fixture schema, before and after: | call | before | after | |---|---|---| | `profile:find("slug", "hero")` | raises "needs #44" | **hit** | | `profile:find("slug", "nope")` | raises "needs #44" | nil | | `profile:find("realm", "eu")` | raises "needs #44" | raises `stdb.cache: table "profile" has no unique column "realm"` | | `message:find("id", 1)` | hit | hit | - **A `find` test on a non-pk unique column.** Every existing `find` test used `message.id`, where pk and unique coincide, so a pk-only `find` passed them all. `log` now declares `unique = { "line" }` — no pk, one unique column, the shape the generator really produces for `profile` — and the suite asserts both a hit and a nil miss on it. - **`db.tables` in the module's own docstrings.** The re-cut renamed every callsite but missed tables.lua's docstrings, which are the API reference: the canonical Defold `init`/`final` block, the `iter` example and the `find` example all still showed the flat form, and each raised `attempt to index field '<name>' (a nil value)` if copied — the exact outcome the module header cites as the reason for the namespace. Also one stale comment in smoke.script. - **The `find` example no longer promises a lookup that cannot work.** It was `db.player:find("identity", me)`, which is precisely the unique-identity lookup #44's README documents as broken by #35 — a special-type column decodes to a fresh wrapper table per row, so neither the identity string nor a re-wrapped copy matches, and it returns a silent nil. The example is now `find("slug", …)`, and the docstring carries the #35 caveat explicitly. - **`err()`'s level was wrong on every path that reaches it.** It hardcoded level 3, documented as "blames the line that called the facade method". `return attach(self, …)` is a tail call, and Lua 5.1 discards the frame level 3 counts on, so `on_insert`/`on_delete`/`on_update` raised with **no file:line at all** — the likeliest real mistake, a typo'd `self.on_row` that is nil, pointed nowhere — while the pk guard blamed `tables.lua` itself. Now level 4, correct for all five, since deleting the seam left `err` reachable only from a helper (`attach`, `require_pk`) that a facade method called. (`off_insert`/`off_delete` take any argument and raise nothing; `detach` scans for a match and a `final()` undoing a registration it never made is not an error.) | | level 3 (shipped) | level 4 (now) | |---|---|---| | `on_insert(nil)` | *(no location at all)* | the caller's line | | `on_delete(nil)` | *(no location at all)* | the caller's line | | `on_update(nil)` | *(no location at all)* | the caller's line | | `log:on_update(fn)` | `stdb/tables.lua:163` | the caller's line | | `log:off_update(fn)` | `stdb/tables.lua:171` | the caller's line | - **Three dispatch contracts that were real but undocumented**, each measured before being written down: - A callback that throws skips everything behind it — the rest of that table's rows *and every later table of the same batch* — while the cache is already complete and `on_event` already ran with the whole batch. A per-table consumer can therefore disagree with an `on_event` consumer about one transaction, and nothing records which rows were dropped. - `db:close()` from inside a callback does not stop the batch: dispatch is a plain loop over already-applied events, so the remaining rows are still delivered. - "Takes effect from the next row" means the next row of the *batch*, not of that table. Register on `log` from a `message` callback and it receives `log`'s rows of that same transaction — rows cached before it existed. - **The bracket form is in the README**, and `require_pk` now records the one residual it has: the guard is only as true as the generated schema, and a stale one inverts it into the silence it exists to prevent. That is #40's "regenerate after any module change" with a sharper consequence, inherited rather than introduced here. ## Verification `6 passed, 0 failed`; stylua clean; native conformance **PASS** first try, both runs, on the rebased tree, with the API exercised end to end: ``` SMOKE: PASS procedure round trip; failure surfaced as an error; per-table API: 1 insert(s), 1 update, 1 delete(s) SMOKE: PASS procedure round trip; failure surfaced as an error; per-table API: 2 insert(s), 1 update, 2 delete(s) [conformance] PASS (identity 0xc200c808... persisted across a process restart) ``` The smoke client registers all three callbacks, calls `find` by pk inside the update callback and asserts it returns the very row the callback was handed, then `off_insert`s from inside that callback — phase 3 proves the detach took by counting the rows that the reconnect replay and the phase-3 query set would otherwise have delivered. Regenerating `chat_schema.lua` off the live server reproduced the committed fixture byte for byte apart from the port in its header comment, so the fixture is current. Falsifiability by mutation — each piece broken in place, suite re-run, then reverted: | mutation | failure | |---|---| | `emit_rows` no longer dispatches | `one call per row, in order, from a subscription: expected insert:1,insert:2, got ` | | dispatch iterates the live list, not the copy | `stdb/tables.lua:266: attempt to call field '?' (a nil value)` | | `detach` stops removing | `a detached callback stops being delivered to: expected 1, got 2` | | `require_pk` made a no-op | `on_update on a pk-less table: expected error, call succeeded` | | callbacks dispatched before `on_event` | `on_event runs before the per-table callbacks: expected event,table, got table,event` | | `find` answers only the pk (the deleted seam, restored) | `stdb.tables: find("line") needs #44` | | `install` skips a name the connection already owns | `test_tables.lua:333: attempt to index field '?' (a nil value)` | The last two are the new coverage: the sixth is the regression the seam deletion closes, and the seventh guards the namespace's totality, which replaced the reserved list. ## Reviewer attention - **`find` is fully index-backed now, with one inherited limit.** A unique `identity` column still does not resolve — `cache.find` keys on the decoded value, and a special type decodes to a fresh table per row (#35). Documented in the docstring and the README row, not worked around here. - **An error raised inside `cache` loses its `file:line` through the facade.** `cache.find`/`iter`/`count` raise at level 2, which lands on the tail call in `Table:find` and prints no prefix. The message still names the table and the column, so it is self-locating by content; fixing it properly means a level argument on the `cache` API, which is #44's surface and already merged. Flagging, not fixing. - **Nothing new is emitted through `on_event`.** A reconnect still empties the cache with no delete callbacks; that is stated in the module header and the README rather than worked around, since `Subscription:on_applied` re-firing is the existing answer.

yuris added 1 commit

2026-08-19 18:20:32 +00:00

tables: a typed accessor per table, callbacks and reads (#27 )

ci / ci (pull_request) Successful in 1m24s

Details

e5f19afd0d

Row changes arrived as one firehose: every consumer wrote the same loop over
`evt.counts.events`, filtering by table name and branching on event type. Every
table in the schema now has an accessor instead - `db.message:on_insert(fn)`,
`:on_update`, `:on_delete`, `:iter`, `:count`, `:find` - which is also where the
generator's column types, pk and unique constraints become usable rather than
merely emitted.

Purely additive. `on_event` is untouched: same events, same order, same
`counts.events`, and it still fires FIRST for every frame - the per-table
callbacks run after it, so nothing an existing consumer already saw can be
reordered by a callback registered behind it. A test replays three frames
through two connections, one with callbacks registered and one without, and
compares the resulting event streams.

A new module rather than more of connection.lua. That file is 750 lines of
request correlation, handles and message dispatch, and a facade shares none of
it: what it needs is the schema and the cache, both of which it takes as
arguments, so tables.lua is pure and unit-testable without an engine.
connection.lua gains one require, one `install` call, and a four-line
`emit_rows`.

Deregistration is the Defold-specific requirement, and it is spelled the way
`Subscription:off_applied` already spells it: `off_insert(fn)`, removing one
registration, the first match. A game object registers in `init(self)` and the
closure holds `self`; if `final(self)` cannot undo that, the first row after the
object is deleted calls into a corpse. A handle returned from `on_insert` would
have been a second idiom for one concept and a second object for that game
object to store, and it would have cost `on_insert` its return value, which
chains instead.

Callbacks fire per row once the WHOLE batch is in the cache, so a callback
reading iter/count/find sees a complete transaction. Each row's callback list is
copied before any of it runs, for the reason connection.lua's `fire` copies its
own: registering or detaching from inside a callback is ordinary - the fire-once
idiom is exactly that - and mutating the list under the loop would either skip a
sibling or index past the end. Attaching and detaching therefore take effect
from the next row. It is one buffer refilled per row rather than one table per
row, so a 500-row transaction does not allocate 500 of them.

`on_update` raises on a pk-less table. Such a table can never report an update -
its row change arrives as a delete plus an insert, and cache.lua deliberately
does not pair those - so the registration could never fire, and accepting it
would turn a wrong belief about the schema into missing output days later.
`cache.find` already raises for the analogous unanswerable lookup.

A table named after something the connection already owns raises at connect.
`db.<table>` puts server-chosen names in the same namespace as `state`, `close`,
`send`, `subscribe`, and the quiet alternative - install the accessors that fit,
skip the rest - is discovered by a consumer inside a game object's `init()` as
`attempt to call method 'on_insert' (a nil value)` on a line copied from the
README, with `db.state` handing back the cache. One table reachable by a
different idiom than every other table is not a thing to freeze at 1.0. No
opt-out: a SpacetimeDB table actually called `state` or `close` is rare enough
that an escape hatch would be permanent surface bought for a hypothetical, and
adding one later is backwards compatible.

Shadowing is impossible regardless of that check. Facades resolve through the
connection's `__index`, behind both its raw fields and its methods, so a raw
field is found before a facade is even considered. The reserved list is tested
against a real connection object, so adding a field or a method to
connection.lua without listing it fails the suite.

`find` delegates to the schema's unique indexes through `cache.find` (#28), which
is not merged yet; until it is, the primary key - the one column whose answer is
identical either way - is answered from `cache.get`, and every other column
raises with the reason rather than silently scanning. Marked SEAM: deleting that
branch is the whole rebase, and no index maintenance is duplicated here.

Conformance exercises the API against a live server: on_insert, on_update and
on_delete registered in the smoke client, `iter` and `find` by pk, and
`off_insert` called from inside the update callback - phase 3 then proves the
detach took, by counting the rows that the reconnect replay and the phase-3
query set would otherwise have delivered.

yuris referenced this pull request

2026-08-19 18:26:34 +00:00

v1.0.0 release checklist #24

yuris force-pushed feat/27-table-api from e5f19afd0d

ci / ci (pull_request) Successful in 1m24s

Details

to 962c715a40

ci / ci (pull_request) Successful in 1m24s

Details

2026-08-19 18:32:06 +00:00

Compare

yuris commented

2026-08-20 01:12:42 +00:00

Author

Owner

Review — automated reviewer, posted for the record

Review — PR #45 `feat/27-table-api` @ `962c715`

Verdict: request changes. The re-cut design is right and I would freeze it. But the two things the author was asked to do were not done, the PR body still describes the rejected design, and the re-cut missed every example inside stdb/tables.lua's own docstrings. All four are mechanical; I verified each fix is safe.

Blocking

B1. The branch is not rebased and the #44 seam is still there

merge-base HEAD origin/main = 6efcfdd  (#37)
in main, not in branch      = 6e37098  (#44)
cache.lua exports on branch = new, apply_database_update, drop_all, drop_table, iter, get, count
                              -> no cache.find

So Table:find takes the if not cache.find fallback. Against the real generated schema (dev/test/fixtures/chat_schema.lua), that means:

table	pk	unique	`find` on PR head
`message`	`id`	`{id}`	`id` HIT; `sender`, `text` raise
`profile`	none	`{slug}`	all 9 columns raise, including `slug`

This is the README mismatch the previous reviewer found and died before describing. README:711 promises:

t:find(column, value) | One row by a single-column unique constraint, pk included; nil on a miss, raises for a column under no unique constraint

profile.slug carries a unique constraint and raises. profile has no pk, so the seam's premise — "the primary key is the one column whose answer is identical either way" — resolves nothing at all for it. On this branch exactly one find call in the whole generated schema works: message:find("id", …).

Verified fixable: rebase is clean, 6/6 green before and after. Deleting the 19-line seam (tables.lua:206-224) leaves return cache.find(self.conn.state, self.name, column, value) — suite 6/6, stylua clean, and profile.slug → HIT, non-unique columns raise with cache.find's own message (stdb.cache: table "profile" has no unique column "realm"). find is then fully index-backed.

Test gap that hid this: no test calls find on a non-pk unique column. Every find test uses message.id, where pk and unique coincide, so the seam's blind spot was invisible. profile:find("slug", …) is one line and would have failed on this branch.

B2. `stdb/tables.lua` docstrings still use the rejected flat form — 5 lines

The re-cut renamed every callsite (smoke.script, README, init.lua) but missed the module's own docstrings, which are the API reference:

line	text	run verbatim
:143, :146	the canonical Defold `init`/`final` example — `db.message:on_insert(self.on_row)` / `off_insert`	`attempt to index field 'message' (a nil value)`
:186	`for row in db.message:iter() do`	`attempt to index field 'message' (a nil value)`
:199	`db.player:find("identity", me)`	`attempt to index field 'player' (a nil value)`

That is the exact error the module header (lines 20-22) cites as the bad outcome the namespace exists to prevent. :143/:146 is the block a Defold user copies wholesale.

Also dev/example/main/smoke.script:341: comment still reads "seen through db.<table>".

(tables.lua:273 "the shorter db.<name>" and the client.db.<table> / ctx.db.<table> peer citations are correct — leave them.)

B3. `tables.lua:199`'s `find` example contradicts #44's README

The only worked find example in the module is db.player:find("identity", me) — precisely the unique-identity lookup main's README (from #44, lines 776-782) documents as not working because of #35. Proven on the rebased tree with a player table keyed by a unique identity column:

db.identity = 0xdeadbeef (type string)
player rows cached: 1
cached row identity field = table: 0x5555555bbe50 (type table)
db.player:find("identity", me) -> ok=true result=nil     <- silent nil
re-wrapped copy                -> ok=true result=nil

Per the docstring's own contract nil means "a genuine miss", so this is exactly the silence the raise elsewhere in the same file exists to eliminate. Use find("slug", …) — the lookup the real schema actually supports — or carry #44's caveat.

Answering the focus question directly: #45's README neither duplicates nor contradicts #44's #35 note; the README is fine. The contradiction is in the module docstring only.

B4. The PR body describes the rejected design

The body still contains the whole ## The name collision section, "A reserved name raises at connect anyway", "No opt-out", "The reserved list is tested against a real connection object", db.message: code blocks, and a Verification mutation row:

mutation	failure
reserved-name check disabled	`a table named "state" is refused: expected error, call succeeded`

That test no longer exists — grep 'is refused' dev/test/ returns nothing. The Reviewer-attention section still says "cache.find is not merged yet" and "The no-opt-out collision raise is the contentious call". The commit message is correct ("under db.tables"); the body is stale and its Verification table is now false.

Non-blocking bug

N1. `err()`'s hardcoded level 3 is correct for exactly one of its three call paths

tables.lua:62-64 claims "Level 3: blames the line that called the facade method, not the method." Measured:

call shape	methods	shipped (level 3)	level 4
method → helper, tail call	`on_insert`, `on_delete`, `off_insert`, `off_delete`	no location at all	caller ✓
method → helper, statement	`on_update`, `off_update` (via `require_pk`)	`tables.lua:163` / `:171` ✗	caller ✓
method → `err` direct	`find`	caller ✓	no location ✗

on_insert(nil)  RAW ERROR = "stdb.tables: callback must be a function, got nil"   <- no file:line
on_update pk-less           -> blames stdb/tables.lua:163, not the caller
find(non-unique)            -> blames the caller  (correct)

Cause: return attach(self, self._insert, fn) is a tail call, so Lua 5.1 discards the frame level 3 counts on. Minimal repro confirms it — breaking the tail call restores the prefix. The likeliest real mistake (a typo'd self.on_row that is nil) therefore raises with zero indication of where. Fix: give err a level argument (4 from attach/detach/require_pk, 3 from find), or route find through a helper too.

Focus-by-focus

1. The namespaced surface — right call, structurally total

db.tables[name] works for every name I could construct: my-table, 2fast, with space, kebab-case-name, end, nil, true, function — all reachable with full facade methods. Dot access behaves as it must: db.tables.2fast is a syntax error; db.tables.my-table compiles (Lua parses it db.tables.my - table) and fails at runtime with an arithmetic error. Loud either way — but the README's per-table section never shows the bracket form, and a hyphenated table name is not exotic. One line worth adding.

No reserved list, no __index closure, no "reserved"/"collision" word left in connection.lua. Connection surface vs origin/main:

+ RAW    tables:table          <- the only difference
__index kind: table            <- both; still a plain table, not a function

Every other raw field and method is identical. db.state is the cache with a table named state/close/send/identity/tables/pending/on_event/subscribe declared; db:close() still resolves; db.tables.tables resolves.

The stated reason holds: a reserved list would make connection.lua's private field names frozen public API, and adding a field would then be a breaking change. Namespacing removes the list entirely. Both peer SDKs namespace. Yes — freeze this.

2. Dispatch and reentrancy — sound, three undocumented frozen contracts

The snapshot design is correct. Per-row refill, one call-local buffer, nesting safe. The nested-dispatch probe (callback feeds another frame from inside row 1) gives A1 A99 B99 B1 A2 B2 A3 B3 — the nested batch runs to completion inside A1 and the outer B still fires for every outer row. No corruption.

A raising callback truncates delivery silently. Row 2's callback throws:

handle_frame ok=false err=boom from row 2
message rows delivered: [1,2]     later rows of same table: NO
log rows delivered:     []        later TABLE: NO
cache: message:count()=3, log:count()=1   <- fully consistent

The cache is complete and on_event already ran with the whole batch. So an on_event consumer and a per-table consumer disagree about the same transaction, and nothing records which rows were dropped. No-pcall is the right default and matches on_event; the header says "the callbacks behind the one that threw do not run" — but does not say the later tables are lost too. Under-documented.

db:close() mid-dispatch does not stop dispatch. Closed after row 1; rows 2 and 3 still delivered. Undocumented. Defensible (the batch completes) but a game that closes on a row keeps receiving rows. One sentence.

Registering on a different table takes effect within the same batch. A callback registered on log from message's row 1 received both log rows of that same transaction — rows already in the cache before the callback existed. Literally consistent with "takes effect from the next row", but the README's framing reads as "not this batch". One clause.

Sibling detach behaves as the snapshot implies and is already covered by the README: B still runs for the current row then never again (A=3, B=1); on a single row, a sibling detached before its turn still runs (X=1, Y=1). Correct — freeze.

3. `on_event` byte-identical — confirmed

Canonical dump of every on_event call and every argument over 6 frames: InitialSubscription across two tables; a transaction mixing one pk-paired update + insert + two deletes across two tables; SubscribeMultiApplied; UnsubscribeMultiApplied; single-query SubscribeApplied; single-query UnsubscribeApplied. Census: 9 inserts, 4 deletes, 1 update, 3 subscribed, 2 unsubscribed, 1 transaction — so it is not passing vacuously.

Three streams — origin/main, PR head with no callbacks, PR head with callbacks on both tables (all three kinds, one reading the cache, one registering another mid-dispatch) — all md5 d85a601b9ec0d5a4052d94e50bc11ff5. Byte-identical. The compatibility claim holds.

4. `on_update` pk-less raise — correct, and safe to freeze

On the real schema: message (pk=id) accepts; profile (pk=nil) raises on both on_update and off_update.

The regeneration hazard is smaller than it looks, and this is the reason: require_pk reads t.def.pk, and cache.lua:154 pairs updates off the same def.pk. So the raise cannot disagree with what dispatch can actually deliver — if the schema says no pk, updates genuinely never arrive and the registration genuinely could never fire. It is derived, not heuristic. That makes it acceptable to freeze.

Residual worth one doc line: under a stale generated schema the guard inverts into the silence it exists to prevent — schema says no pk while the server has one → on_update raises though updates would flow; schema says pk while the server dropped it → the cache pairs on a now-absent column. That is #40's "regenerate after any module change" with a sharper consequence, inherited from #34/#44 rather than introduced here.

A table losing its pk server-side turns a game object's init() into a boot crash. That is the intended loud failure; keep it.

5. README `find` vs the real schema

See B1 and B3. Real constraints: message pk=id unique={id}; profile pk=none unique={slug} plus a composite unique index (realm+handle) that find cannot serve — the README's "single-column" wording covers that correctly, no change needed there.

6. Cost — dispatch is not the problem

tables.dispatch in isolation, 20k rows, wall clock + deterministic VM instruction count:

case	ms/batch	VM instrs
insert ×20k, 0 callbacks	0.865	261,000
insert ×20k, 1 cb/table	2.153	722,000
insert ×20k, 3 cb/table	3.550	1,164,000
update ×20k, 0 callbacks	1.727	261,000
update ×20k, 1 cb	3.393	702,000
mixed ×20k, 0 callbacks	0.797	261,000
table has no facade (floor)	0.555	160,000

The "no per-row buffer work when nobody is listening" claim is verified — and the instruction count proves it rather than suggesting it. 261,000 is identical for inserts, updates and mixed at zero callbacks, so execution never reaches the e.type == "update" branch: it exits at if n > 0 every row. 13.05 instr/row with no listeners vs 36.1 with one. Facade lookup + LIST[e.type] + #list costs ~5 instr/row over the no-facade floor.

For scale: handle_frame of the same 20k-row batch end to end is 180.2 ms, so dispatch with no listeners is 0.48% of it. #30's frame budget is a decode / apply_database_update problem; this PR adds nothing meaningful to it. Flagging, not asking for a fix.

7. Conformance and the guard — both confirmed

Rebased tree, own server (STDB_PORT=3800, STDB_STATE_DIR=/tmp/stdb-45review): PASS both runs, first try. No #39 flake.

SMOKE: PASS procedure round trip; failure surfaced as an error; per-table API: 1 insert(s), 1 update, 1 delete(s)
SMOKE: PASS ... per-table API: 2 insert(s), 1 update, 2 delete(s)
[conformance] PASS (identity 0xc2000da5... persisted across a process restart)

Fixture check: dev-server.sh regenerated chat_schema.lua and only the port in the header comment differed — the fixture is current, not stale.

The end-to-end guard is real (this is the part nobody had confirmed). tables.dispatch → no-op:

unit:        FAIL one call per row, in order, from a subscription: expected insert:1,insert:2, got 
conformance: SMOKE: FAIL on_update fired 0 time(s) for one pk change      -> exit 1

And the subtler deregistration assertion is also real — detach → no-op:

unit:        FAIL a detached callback stops being delivered to: expected 1, got 2
conformance: SMOKE: FAIL off_insert did not stop delivery: 2 inserts now, 1 when detached   -> exit 1

Did the re-cut shrink the diff and change nothing else?

Recovered the pre-re-cut head e5f19af (same base 6efcfdd) from the previous reviewer's clone, so this is a direct comparison.

file	pre-re-cut	re-cut	Δ
`stdb/tables.lua`	+323	+289	−34
`stdb/connection.lua`	+41	+31	−10
`README.md`	+79	+87	+8
`dev/test/test_tables.lua`	+403	+415	+12
`stdb/init.lua`	+10	+12	+2
`dev/example/main/smoke.script`	+70	+70	0
total	+918	+896	−22

Net shrink, as promised. The code-only delta is exactly what was claimed and nothing more:

M.reserved (30 entries) deleted; the collision error(...) deleted; c._tables → c.tables. Every other tables.lua delta line is a comment.
connection.lua: the __index = function(_, k) return M[k] or facades[k] end closure and the second setmetatable deleted, replaced by one c.tables = tables.install(c). This also drops a redundant double metatable set and restores __index to a table — one fewer closure call per method miss.
Tests: the 7 collision cases inverted from "is refused" to "is reachable as db.tables.X"; the reserved-list drift loop (for k in pairs(probe) ... tables.reserved[k]) removed, correctly — there is no list left to drift. Added db.tables.tables resolves, db.state is the cache even with a table named state, and the facade reads that same cache, db:close() still resolves.
smoke.script delta is a pure db.X → db.tables.X rename (6 lines).

The README (+8) and test (+12) growth is the namespace argument and the new totality cases — justified, not scope creep. Nothing unrelated moved.

Summary

The design is right: namespaced accessors, snapshot-per-row dispatch, on_event untouched and provably byte-identical, on_update gated on the same def.pk the cache pairs on. The re-cut did what it said. Blocking work is B1 (rebase + delete the 19-line seam), B2 (5 flat-form docstring lines + 1 smoke comment), B3 (the identity find example), B4 (rewrite the body) — all mechanical, and I verified the rebase is clean, the seam deletion is safe, and the suite plus conformance stay green through both. Fold in N1 and the three focus-2 doc gaps while the file is open, and add profile:find("slug", …) as the test that would have caught B1.

Verdict: REQUEST CHANGES — design is right and I would freeze it; 4 blocking items, all mechanical, all verified fixable

## Review — automated reviewer, posted for the record # Review — PR #45 `feat/27-table-api` @ `962c715` **Verdict: request changes.** The re-cut design is right and I would freeze it. But the two things the author was asked to do were not done, the PR body still describes the *rejected* design, and the re-cut missed every example inside `stdb/tables.lua`'s own docstrings. All four are mechanical; I verified each fix is safe. --- ## Blocking ### B1. The branch is not rebased and the #44 seam is still there ``` merge-base HEAD origin/main = 6efcfdd (#37) in main, not in branch = 6e37098 (#44) cache.lua exports on branch = new, apply_database_update, drop_all, drop_table, iter, get, count -> no cache.find ``` So `Table:find` takes the `if not cache.find` fallback. Against the **real generated schema** (`dev/test/fixtures/chat_schema.lua`), that means: | table | pk | unique | `find` on PR head | |---|---|---|---| | `message` | `id` | `{id}` | `id` **HIT**; `sender`, `text` raise | | `profile` | *none* | `{slug}` | **all 9 columns raise, including `slug`** | **This is the README mismatch the previous reviewer found and died before describing.** README:711 promises: > `t:find(column, value)` | One row by a single-column unique constraint, `pk` included; nil on a miss, raises for a column under no unique constraint `profile.slug` carries a unique constraint and **raises**. `profile` has no pk, so the seam's premise — "the primary key is the one column whose answer is identical either way" — resolves *nothing at all* for it. On this branch exactly one `find` call in the whole generated schema works: `message:find("id", …)`. Verified fixable: rebase is clean, 6/6 green before and after. Deleting the 19-line seam (`tables.lua:206-224`) leaves `return cache.find(self.conn.state, self.name, column, value)` — suite 6/6, stylua clean, and `profile.slug` → **HIT**, non-unique columns raise with `cache.find`'s own message (`stdb.cache: table "profile" has no unique column "realm"`). `find` is then fully index-backed. **Test gap that hid this:** no test calls `find` on a non-pk unique column. Every find test uses `message.id`, where pk and unique coincide, so the seam's blind spot was invisible. `profile:find("slug", …)` is one line and would have failed on this branch. ### B2. `stdb/tables.lua` docstrings still use the rejected flat form — 5 lines The re-cut renamed every *callsite* (smoke.script, README, init.lua) but missed the module's own docstrings, which are the API reference: | line | text | run verbatim | |---|---|---| | :143, :146 | the canonical Defold `init`/`final` example — `db.message:on_insert(self.on_row)` / `off_insert` | `attempt to index field 'message' (a nil value)` | | :186 | `for row in db.message:iter() do` | `attempt to index field 'message' (a nil value)` | | :199 | `db.player:find("identity", me)` | `attempt to index field 'player' (a nil value)` | That is the *exact* error the module header (lines 20-22) cites as the bad outcome the namespace exists to prevent. :143/:146 is the block a Defold user copies wholesale. Also `dev/example/main/smoke.script:341`: comment still reads "seen through `db.<table>`". (`tables.lua:273` "the shorter `db.<name>`" and the `client.db.<table>` / `ctx.db.<table>` peer citations are correct — leave them.) ### B3. `tables.lua:199`'s `find` example contradicts #44's README The only worked `find` example in the module is `db.player:find("identity", me)` — precisely the unique-identity lookup main's README (from #44, lines 776-782) documents as **not working** because of #35. Proven on the rebased tree with a `player` table keyed by a unique identity column: ``` db.identity = 0xdeadbeef (type string) player rows cached: 1 cached row identity field = table: 0x5555555bbe50 (type table) db.player:find("identity", me) -> ok=true result=nil <- silent nil re-wrapped copy -> ok=true result=nil ``` Per the docstring's own contract nil means "a genuine miss", so this is exactly the silence the raise elsewhere in the same file exists to eliminate. Use `find("slug", …)` — the lookup the real schema actually supports — or carry #44's caveat. **Answering the focus question directly:** #45's *README* neither duplicates nor contradicts #44's #35 note; the README is fine. The contradiction is in the module docstring only. ### B4. The PR body describes the rejected design The body still contains the whole `## The name collision` section, "**A reserved name raises at `connect` anyway**", "No opt-out", "The reserved list is tested against a real connection object", `db.message:` code blocks, and a Verification mutation row: | mutation | failure | |---|---| | reserved-name check disabled | `a table named "state" is refused: expected error, call succeeded` | That test no longer exists — `grep 'is refused' dev/test/` returns nothing. The Reviewer-attention section still says "`cache.find` is not merged yet" and "The no-opt-out collision raise is the contentious call". The commit message is correct ("under db.tables"); the body is stale and its Verification table is now false. --- ## Non-blocking bug ### N1. `err()`'s hardcoded level 3 is correct for exactly one of its three call paths `tables.lua:62-64` claims *"Level 3: blames the line that called the facade method, not the method."* Measured: | call shape | methods | shipped (level 3) | level 4 | |---|---|---|---| | method → helper, **tail call** | `on_insert`, `on_delete`, `off_insert`, `off_delete` | **no location at all** | caller ✓ | | method → helper, statement | `on_update`, `off_update` (via `require_pk`) | `tables.lua:163` / `:171` ✗ | caller ✓ | | method → `err` direct | `find` | caller ✓ | no location ✗ | ``` on_insert(nil) RAW ERROR = "stdb.tables: callback must be a function, got nil" <- no file:line on_update pk-less -> blames stdb/tables.lua:163, not the caller find(non-unique) -> blames the caller (correct) ``` Cause: `return attach(self, self._insert, fn)` is a **tail call**, so Lua 5.1 discards the frame level 3 counts on. Minimal repro confirms it — breaking the tail call restores the prefix. The likeliest real mistake (a typo'd `self.on_row` that is nil) therefore raises with zero indication of where. Fix: give `err` a level argument (4 from `attach`/`detach`/`require_pk`, 3 from `find`), or route `find` through a helper too. --- ## Focus-by-focus ### 1. The namespaced surface — right call, structurally total `db.tables[name]` works for every name I could construct: `my-table`, `2fast`, `with space`, `kebab-case-name`, `end`, `nil`, `true`, `function` — all reachable with full facade methods. Dot access behaves as it must: `db.tables.2fast` is a syntax error; `db.tables.my-table` **compiles** (Lua parses it `db.tables.my - table`) and fails at runtime with an arithmetic error. Loud either way — but the README's per-table section never shows the bracket form, and a hyphenated table name is not exotic. One line worth adding. No reserved list, no `__index` closure, no "reserved"/"collision" word left in connection.lua. Connection surface vs `origin/main`: ``` + RAW tables:table <- the only difference __index kind: table <- both; still a plain table, not a function ``` Every other raw field and method is identical. `db.state` is the cache with a table named `state`/`close`/`send`/`identity`/`tables`/`pending`/`on_event`/`subscribe` declared; `db:close()` still resolves; `db.tables.tables` resolves. The stated reason holds: a reserved list would make connection.lua's private field names frozen public API, and adding a field would then be a breaking change. Namespacing removes the list entirely. Both peer SDKs namespace. Yes — freeze this. ### 2. Dispatch and reentrancy — sound, three undocumented frozen contracts The snapshot design is correct. Per-row refill, one call-local buffer, nesting safe. The nested-dispatch probe (callback feeds another frame from inside row 1) gives `A1 A99 B99 B1 A2 B2 A3 B3` — the nested batch runs to completion inside A1 and the outer B still fires for every outer row. No corruption. **A raising callback truncates delivery silently.** Row 2's callback throws: ``` handle_frame ok=false err=boom from row 2 message rows delivered: [1,2] later rows of same table: NO log rows delivered: [] later TABLE: NO cache: message:count()=3, log:count()=1 <- fully consistent ``` The cache is complete and `on_event` already ran with the whole batch. So an `on_event` consumer and a per-table consumer disagree about the same transaction, and nothing records which rows were dropped. No-pcall is the right default and matches `on_event`; the header says "the callbacks behind the one that threw do not run" — but does **not** say the later *tables* are lost too. Under-documented. **`db:close()` mid-dispatch does not stop dispatch.** Closed after row 1; rows 2 and 3 still delivered. Undocumented. Defensible (the batch completes) but a game that closes on a row keeps receiving rows. One sentence. **Registering on a *different* table takes effect within the same batch.** A callback registered on `log` from `message`'s row 1 received both `log` rows of that same transaction — rows already in the cache before the callback existed. Literally consistent with "takes effect from the next row", but the README's framing reads as "not this batch". One clause. Sibling detach behaves as the snapshot implies and is already covered by the README: B still runs for the current row then never again (A=3, B=1); on a single row, a sibling detached before its turn **still runs** (X=1, Y=1). Correct — freeze. ### 3. `on_event` byte-identical — confirmed Canonical dump of every `on_event` call and every argument over 6 frames: InitialSubscription across two tables; a transaction mixing one pk-paired update + insert + two deletes across two tables; SubscribeMultiApplied; UnsubscribeMultiApplied; single-query SubscribeApplied; single-query UnsubscribeApplied. Census: 9 inserts, 4 deletes, 1 update, 3 subscribed, 2 unsubscribed, 1 transaction — so it is not passing vacuously. Three streams — `origin/main`, PR head with **no** callbacks, PR head with callbacks on **both** tables (all three kinds, one reading the cache, one registering another mid-dispatch) — all md5 **`d85a601b9ec0d5a4052d94e50bc11ff5`**. Byte-identical. The compatibility claim holds. ### 4. `on_update` pk-less raise — correct, and safe to freeze On the real schema: `message` (pk=`id`) accepts; `profile` (pk=nil) raises on both `on_update` and `off_update`. The regeneration hazard is smaller than it looks, and this is the reason: `require_pk` reads `t.def.pk`, and `cache.lua:154` pairs updates off **the same** `def.pk`. So the raise cannot disagree with what dispatch can actually deliver — if the schema says no pk, updates genuinely never arrive and the registration genuinely could never fire. It is derived, not heuristic. That makes it acceptable to freeze. Residual worth one doc line: under a **stale** generated schema the guard inverts into the silence it exists to prevent — schema says no pk while the server has one → `on_update` raises though updates would flow; schema says pk while the server dropped it → the cache pairs on a now-absent column. That is #40's "regenerate after any module change" with a sharper consequence, inherited from #34/#44 rather than introduced here. A table losing its pk server-side turns a game object's `init()` into a boot crash. That is the intended loud failure; keep it. ### 5. README `find` vs the real schema See B1 and B3. Real constraints: `message` pk=`id` unique=`{id}`; `profile` pk=none unique=`{slug}` plus a **composite** unique index `(realm+handle)` that `find` cannot serve — the README's "single-column" wording covers that correctly, no change needed there. ### 6. Cost — dispatch is not the problem `tables.dispatch` in isolation, 20k rows, wall clock + deterministic VM instruction count: | case | ms/batch | VM instrs | |---|---|---| | insert ×20k, **0 callbacks** | 0.865 | **261,000** | | insert ×20k, 1 cb/table | 2.153 | 722,000 | | insert ×20k, 3 cb/table | 3.550 | 1,164,000 | | **update** ×20k, 0 callbacks | 1.727 | **261,000** | | update ×20k, 1 cb | 3.393 | 702,000 | | **mixed** ×20k, 0 callbacks | 0.797 | **261,000** | | table has no facade (floor) | 0.555 | 160,000 | **The "no per-row buffer work when nobody is listening" claim is verified — and the instruction count proves it rather than suggesting it.** 261,000 is *identical* for inserts, updates and mixed at zero callbacks, so execution never reaches the `e.type == "update"` branch: it exits at `if n > 0` every row. 13.05 instr/row with no listeners vs 36.1 with one. Facade lookup + `LIST[e.type]` + `#list` costs ~5 instr/row over the no-facade floor. For scale: `handle_frame` of the same 20k-row batch end to end is **180.2 ms**, so dispatch with no listeners is **0.48%** of it. #30's frame budget is a decode / `apply_database_update` problem; this PR adds nothing meaningful to it. Flagging, not asking for a fix. ### 7. Conformance and the guard — both confirmed Rebased tree, own server (`STDB_PORT=3800`, `STDB_STATE_DIR=/tmp/stdb-45review`): **PASS both runs, first try.** No #39 flake. ``` SMOKE: PASS procedure round trip; failure surfaced as an error; per-table API: 1 insert(s), 1 update, 1 delete(s) SMOKE: PASS ... per-table API: 2 insert(s), 1 update, 2 delete(s) [conformance] PASS (identity 0xc2000da5... persisted across a process restart) ``` Fixture check: `dev-server.sh` regenerated `chat_schema.lua` and only the port in the header comment differed — the fixture is current, not stale. **The end-to-end guard is real** (this is the part nobody had confirmed). `tables.dispatch` → no-op: ``` unit: FAIL one call per row, in order, from a subscription: expected insert:1,insert:2, got conformance: SMOKE: FAIL on_update fired 0 time(s) for one pk change -> exit 1 ``` And the subtler **deregistration** assertion is also real — `detach` → no-op: ``` unit: FAIL a detached callback stops being delivered to: expected 1, got 2 conformance: SMOKE: FAIL off_insert did not stop delivery: 2 inserts now, 1 when detached -> exit 1 ``` --- ## Did the re-cut shrink the diff and change nothing else? Recovered the pre-re-cut head **`e5f19af`** (same base `6efcfdd`) from the previous reviewer's clone, so this is a direct comparison. | file | pre-re-cut | re-cut | Δ | |---|---|---|---| | `stdb/tables.lua` | +323 | +289 | **−34** | | `stdb/connection.lua` | +41 | +31 | **−10** | | `README.md` | +79 | +87 | +8 | | `dev/test/test_tables.lua` | +403 | +415 | +12 | | `stdb/init.lua` | +10 | +12 | +2 | | `dev/example/main/smoke.script` | +70 | +70 | 0 | | **total** | **+918** | **+896** | **−22** | Net shrink, as promised. The **code-only** delta is exactly what was claimed and nothing more: - `M.reserved` (30 entries) deleted; the collision `error(...)` deleted; `c._tables` → `c.tables`. Every other `tables.lua` delta line is a comment. - `connection.lua`: the `__index = function(_, k) return M[k] or facades[k] end` closure and the second `setmetatable` deleted, replaced by one `c.tables = tables.install(c)`. This also drops a redundant double metatable set and restores `__index` to a table — one fewer closure call per method miss. - Tests: the 7 collision cases inverted from "is refused" to "is reachable as `db.tables.X`"; the reserved-list drift loop (`for k in pairs(probe) ... tables.reserved[k]`) removed, correctly — there is no list left to drift. Added `db.tables.tables resolves`, `db.state is the cache even with a table named state`, `and the facade reads that same cache`, `db:close() still resolves`. - `smoke.script` delta is a pure `db.X` → `db.tables.X` rename (6 lines). The README (+8) and test (+12) growth is the namespace argument and the new totality cases — justified, not scope creep. **Nothing unrelated moved.** --- ## Summary The design is right: namespaced accessors, snapshot-per-row dispatch, `on_event` untouched and provably byte-identical, `on_update` gated on the same `def.pk` the cache pairs on. The re-cut did what it said. Blocking work is B1 (rebase + delete the 19-line seam), B2 (5 flat-form docstring lines + 1 smoke comment), B3 (the identity `find` example), B4 (rewrite the body) — all mechanical, and I verified the rebase is clean, the seam deletion is safe, and the suite plus conformance stay green through both. Fold in N1 and the three focus-2 doc gaps while the file is open, and add `profile:find("slug", …)` as the test that would have caught B1. --- **Verdict:** REQUEST CHANGES — design is right and I would freeze it; 4 blocking items, all mechanical, all verified fixable

yuris referenced this pull request

2026-08-20 01:20:24 +00:00

protocol: decode every AlgebraicType from the schema tags #46

yuris force-pushed feat/27-table-api from 962c715a40

ci / ci (pull_request) Successful in 1m24s

Details

to 462d96bfe3

ci / ci (pull_request) Successful in 19s

Details

2026-08-20 01:26:28 +00:00

Compare

yuris force-pushed feat/27-table-api from 462d96bfe3