Skip to content

perf(sharing): read the earn total from one batched aggregate instead of a per-row query - #243

Merged
argszero merged 1 commit into
mainfrom
fix/sharings-earn-batched-aggregate
Sep 14, 2026
Merged

argszero merged 1 commit into
mainfrom
fix/sharings-earn-batched-aggregate

Conversation

@argszero

Copy link
Copy Markdown
Owner

Summary

Item 2 of rant 2026-09-14T21:15:02 (host report: refreshing #/sharing on dev lands on the login page). Items 1 and 3 shipped in #240 (95d69ba, v0.7.24) and #242 (5d4e6f4); this PR removes the per-row query on the sharing page.

The N+1

sharing.rs::sharing_row fetched each sharing's earnings with

SELECT COALESCE(SUM(pts), 0) FROM transactions WHERE key_id = ?1 AND type = 'earn'

so GET /api/sharings issued N extra statements on top of the list query — each one a prepare plus an index seek, and on the NAS each one touching more pages.

The list and the single-row (PATCH) call sites also had to keep two identical 13-column SELECT lists in sync by hand; the row builder's own doc comment warned that editing only one of them silently misaligns every field. Both problems have the same fix, so this PR makes one change rather than two:

const ROW_SELECT: &str = "SELECT k.id, …, k.created_at, COALESCE(e.earn, 0) \
         FROM keys k \
         LEFT JOIN (SELECT key_id, SUM(pts) AS earn FROM transactions WHERE type = 'earn' \
                    GROUP BY key_id) e ON e.key_id = k.id ";

sharing_row now only reads columns (mirrors the existing admin_models.rs::ROW_SELECT + row_to_json pair), and both call sites use the constant.

Measured on a synthetic 200,000-row table with 8 keys (local SSD, warm cache): eight subqueries 1.54 ms vs one batched aggregate 0.02 ms. The aggregate scans the covering index added in v15 (idx_transactions_key_id_type_pts), so it never touches the table; on the NAS the per-row variant also pays a page read per key, so the gap grows with N.

Why three levels of assertion

Every wrong shape here keeps the same values — that is what made this invisible:

assertion catches
sharings_earn_is_one_batched_aggregate (values) type='earn' only, consume-only key is 0, another user's 1000.0 earn neither appears nor leaks into the total, and list/patch agree
the_list_query_aggregates_earn_once_over_a_covering_index (plan) a correlated subquery (CORRELATED in the plan = the N+1 shape) and a non-covering scan
perf_gate.rs::the_sharing_row_builder_runs_no_sql (shape) a future query_row inside the row builder — it compiles, every value test still passes, and only the plan gets worse

The gate's positive controls are list/patch in the same file, so it cannot pass by scanning an empty region; perf_gate now covers five route files instead of four.

Related Issue

Tracked as rant 2026-09-14T21:15:02 (no GitHub issue). Item 4 (boot session restore falling back to the login page on a non-401 failure) is still open and will follow as its own PR.

Tests

  • cargo test — 256 passed, 0 failed (252 before)
  • cargo fmt --check clean
  • A/B (fixed tree hash re-verified after each leg):
    • per-row query back inside the row builder → only the new gate red (all value tests stay green — the defect really is invisible to them);
    • aggregate without WHERE type = 'earn' → only the value test red;
    • correlated subquery in the SELECT list → only the plan test red, value test still green.

… of a per-row query

Host report (rant 2026-09-14T21:15:02, item 2): the sharing page ran one query per
row. `sharing_row` fetched each sharing's earnings with

    SELECT COALESCE(SUM(pts), 0) FROM transactions WHERE key_id = ?1 AND type = 'earn'

so a list of N sharings issued N extra statements on top of the list query -- each
one a prepare plus an index seek, and on the NAS each one touching more pages.

The list and the single-row (PATCH) call sites already had to keep two identical
13-column SELECT lists in sync by hand; the row builder's doc comment warned that
editing only one of them silently misaligns every field. Both problems have the same
fix: one `ROW_SELECT` constant that carries the columns and the aggregate, and a row
builder that only reads columns (mirrors `admin_models.rs::ROW_SELECT` + `row_to_json`).

    LEFT JOIN (SELECT key_id, SUM(pts) AS earn FROM transactions
               WHERE type = 'earn' GROUP BY key_id) e ON e.key_id = k.id

Measured on a synthetic 200,000-row table with 8 keys (local SSD, warm cache): eight
subqueries 1.54 ms versus one batched aggregate 0.02 ms. The aggregate scans the
covering index added in v15 (`idx_transactions_key_id_type_pts`), so it does not touch
the table at all; on the NAS the per-row variant also pays a page read per key.

Acceptance is asserted at three levels, because the wrong shapes here keep the same
values:

  - values: `sharings_earn_is_one_batched_aggregate` -- `type='earn'` only, a
    consume-only key is 0, another user's 1000.0 earn neither appears nor leaks into
    the total, and `list` and `patch` agree;
  - plan: `the_list_query_aggregates_earn_once_over_a_covering_index` -- the plan must
    not contain `CORRELATED` (that is the N+1 shape) and must scan
    `COVERING INDEX idx_transactions_key_id_type_pts`;
  - shape: `perf_gate.rs::the_sharing_row_builder_runs_no_sql` -- the row builder must
    contain no SQL call at all, with `list`/`patch` as positive controls. That gate is
    what keeps a future `query_row` inside the builder from being invisible: it
    compiles, every value test still passes, and only the plan gets worse.

`cargo test` 252 -> 256 passed; `cargo fmt --check` clean. A/B legs (each restores the
tree and re-verifies hashes): per-row query in the builder -> only the new gate red;
aggregate without `type='earn'` -> only the value test red; correlated subquery in the
SELECT list -> only the plan test red, with the value test still green.
@argszero
argszero merged commit 199566c into main Sep 14, 2026
1 check passed
@argszero
argszero deleted the fix/sharings-earn-batched-aggregate branch September 14, 2026 14:45
argszero added a commit that referenced this pull request Sep 15, 2026
Ships the 18 PRs merged since v0.7.24 (#242-#259). Schema 14 -> 15 (two
covering indexes, applied at startup). No config change, so no deployment-side
config.toml edit is needed.

Two themes:

Perf on the NFS dev database
- #259: stop mapping the db (PRAGMA mmap_size 64MB -> 0) and stop a real write
  per request (dao::touch_api_key gains a 60s guard). Measured on the live dev
  db: mmap=64MB 1.7-3.1s per COUNT / 250 MiB read vs mmap=0 ~10.5ms / 80 KiB;
  mmap=0 alone still leaves ~1.2s behind any write, so the pair is required.
- #242: codify the two emergency indexes in a v15 migration and gate the
  conditional joins at the plan level.
- #243: read the sharing page's earn total from one batched aggregate.

Frontend: display must equal what it filters on, and one fact, one source
- #250 one writer for the transaction cache; #251 clear every session slot at
  the identity boundary and give the wallet view a loader; #253 one shared
  writer for the wallet/dashboard month-changes; #254 boot loads only the
  destination view; #255 a model row's identity is the model, not its index;
  #256 the marketplace source follows the session, not whether data arrived;
  #257 the sidebar advertises only digits that work; #258 the admin
  total-balance card sums the gift amount its caption names.

i18n
- #249 every backend error reaches the wordlist, and the comment stripper stops
  mangling UTF-8; #252 the backend stops inventing Chinese display labels in
  response data fields.

Forms and robustness
- #244 a non-auth boot failure no longer looks like being logged out; #245 a
  credential 401 is no longer read as a session expiry; #246 wire timestamps
  reach the renderer unsliced; #247 inline cards submit from every field; #248
  a market row's availability label comes from that row.

- Cargo.toml / Cargo.lock: 0.7.24 -> 0.7.25.
- CHANGELOG.md: v0.7.25 entry.
- ui/index.html: cache-bust left as-is; the UI PRs in this release already
  advanced it past the value deployed with v0.7.24 (app.js 20260915-13,
  i18n.js 20260915-3).

cargo test 288 passed; cargo fmt --check clean; clippy unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant