improvement(tables): stop re-reading values the request already holds - #8107
Merged
Merged
Conversation
Two round trips a table request paid for data it was already carrying: - `checkAccess` — the gate every raw `/api/table/**` route shares — resolved the caller's workspace access, then asked the capability check for `tables.use` without telling it which organization owns the workspace. The resolver looks that up itself when it is omitted, so the workspace was read twice per request. It now goes through `checkWorkspaceAccess`, which `getUserEntityPermissions` already delegates to, and passes the organization it hands back. Same single resolution, one fewer read, on views, dispatches, export, import, columns, metadata and the rest. - An appending insert asked for `max(order_key)` and `max(position)` as two statements, both inside the row-order advisory lock every other inserting request is queued behind. Postgres plans each `max()` as its own InitPlan, so one statement still serves each from its own index — the same two index-only backward scans, in one round trip instead of two. Statements per request: views 7→6, dispatches 8→7, export 14→13, insert 14→13.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
|
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #8104. Two more round trips a table request was paying for data it already held:
checkAccessread the workspace twice. It resolves the caller's workspace access, then asks the capability check whether the group withholdstables.use— without telling it which organization owns the workspace.resolvePermissionGroupConfiglooks that up itself when the argument is omitted (its own docs: "Everything else holds the organization id already and should pass it, rather than paying for a second lookup of a value it has"), so the workspace was read a second time on every raw/api/table/**route. It now resolves throughcheckWorkspaceAccess— whichgetUserEntityPermissionsalready delegates to, so it is the same single resolution — and passes the organization that comes back with it. Both arms of the resolver end in the sameresolveUserAccessControlContextForOrganizationcall and agree on every branch, including the non-hosted short-circuit.max(order_key)andmax(position)were two statements, both issued inside the row-order advisory lock that every other inserting request on that table is queued behind. Postgres plans eachmax()as its own InitPlan, so one statement still serves each aggregate from its own index — verified identical plans, two index-only backward scans, 4 buffers each — in one round trip instead of two.Statements per request, from the Postgres statement log (two runs, deterministic):
GET viewsGET dispatchesGET exportPOST rows(append insert)Two things I did NOT do, and why
Both were on the follow-up list in #8104. Measuring them killed them:
count(*) OVER ()to fold the total into the page query. Actively harmful. The window is evaluated beforeLIMIT, so it forces the whole match set through a sort. Measured on a 100k-row table: an index scan reading 11 buffers in 0.23ms becomes a sequential scan + sort of all 100,000 rows in 144ms, spilling 20,864 temp buffers. A 630× regression on the module's hottest read.Benchmarks
Same method as #8104: production build, real HTTP, interleaved A/B between two prebuilt trees, p50 over 3 rounds × 20 iterations, loopback shim adding a fixed 2ms DB round trip.
Untouched endpoints (
rows,table,tables-list) are flat within noise, which is the regression check. Modest by design — this is one round trip each, not a structural change.Behavioural equivalence
Captured every table endpoint's exact body against both trees across four table sizes — 59 entries, 84,571,541 bytes — plus a row-ordering exercise driving append,
afterRowId,beforeRowId, explicitposition, and both rejection cases, then reading the resulting visual order back. Identical apart from generated row ids and oneupdatedAtthat the capture's own inserts bumped. Ordering came back the same on both: appends take consecutive positions, anchored inserts land beside their anchor, positional inserts land at the requested slot, conflicting anchors are still400, an unknown anchor is still404.Type of Change
Testing
vitest runfull app suite: 54,564 passed, 1 pre-existing load-flake (an unrelated UI test that passes in isolation)nullfor a personal workspace) rather thanundefined, and that the append reads both anchors in a single select — and each was verified to fail when its change is revertedbun run check:audits(47 audits),bun run lint,bun run type-check,docs-manifest:check, block-registry check — all passEXPLAIN (ANALYZE, BUFFERS)Checklist