Skip to content

fix(db): stop mapping the DB on this NFS and stop a real write per request - #259

Merged
argszero merged 1 commit into
mainfrom
fix/nfs-mmap-and-per-request-write
Sep 15, 2026
Merged

argszero merged 1 commit into
mainfrom
fix/nfs-mmap-and-per-request-write

Conversation

@argszero

Copy link
Copy Markdown
Owner

Summary

On dev, /api/transactions?page=1&page_size=1 takes 1.7-2.3s and /api/ops/runtime 2.1s
(rant 2026-09-15T10:33:34). Two independent NFS mechanisms stack up, and this PR fixes both —
neither is worth much alone.

Defect 1db.rs::open() sets PRAGMA mmap_size=67108864 (v12: "64MB mmap readahead, the whole
file resident in the process, remote storage read only once"). That claim is false on this NFS, and I
measured it on the live dev database (149.6 MiB, 373,322 transactions, busiest user 186,665 rows)
with a read-only connection at the app's own cache_size=-65536, running the app's own statement:

SELECT COUNT(*) FROM transactions WHERE user_id=2      # idx_transactions_user_id
  mmap=64MB : 3144.6 / 2807.7 / 2894.0 / 1970.5 / 1812.2 ms  | read_bytes +262,438,912 (250 MiB)
  mmap=    0:   32.8 /   10.8 /   10.6 /   10.4 /   10.5 ms  | read_bytes +     81,920 ( 80 KiB)
  (repeated, interleaved, same numbers)

The mapped pages never make it into the NFS client cache, so every query really reads the file; the
pread path can use the client cache plus readahead. It is not "slow storage": the same file
copied to a local disk answers in 7ms under both settings. A size sweep (0 → 13ms, 16MB → 334-407ms,
64MB → 1.7-2.2s, 128MB → 2.2-2.8s) says every non-zero value is worse, so the fix is 0, not "a bigger
number".

Defect 2 — every authenticated request goes through dao::touch_api_key
(routes/mod.rs:135, inside AuthUser::from_request_parts) and writes last_used. A write that
changes the file drops the client's cached pages, so defect 1's fix alone still leaves ~1.2s behind
every write. Measured on a throwaway copy of the dev DB in the same NFS directory (deleted
afterwards), reader already warm at mmap=0:

reader warm baseline          : 10.8 10.8 10.8 ms
unguarded write  changed=1 commit=34.2ms -> reader 1166.5 12.5 10.7 10.7 10.7 ms   # cache dropped
reader re-warmed              : 10.9 10.7 10.5 ms
guarded, in window changed=0 commit= 7.2ms -> reader   10.9 12.5 11.3 10.9 10.2 ms # cache survives
guarded, elapsed   changed=1 commit=37.5ms -> reader 1225.5 11.5 10.7 10.9 10.9 ms # once/min

last_used only feeds the "last used" column of the key list, where minute resolution is plenty, so
the guard drops a real write per request to at most one per minute. SQLite does not write a page for
an UPDATE that matches no rows — hence 7ms and an untouched cache.

Related Issue

No issue; the work order is host rant 2026-09-15T10:33:34 (quoted above).

Changes

  • src/db.rs: mmap_size 671088640; the doc comment keeps v12's page-cache half
    (cache_size=-65536) with its rationale, the measurement and the size sweep, and corrects the
    falsified "readahead / whole file resident / read only once" claim that v12 wrote
    (the rant asks
    for that comment to be fixed in the same change).
  • src/dao.rs: touch_api_key gains the time guard
    AND (last_used IS NULL OR last_used < datetime('now','-60 seconds')), with a doc comment naming
    its caller and the measurement.
  • New gate tests: db::tests::open_does_not_enable_memory_mapping (mmap must stay 0 — a regression
    here puts the 1.5-1.8s cold read straight back — plus a positive control that the 64MB page cache
    is not removed with it) and
    dao::tests::touch_api_key_writes_at_most_once_per_guard_window (NULL writes; in-window calls
    must match 0 rows; elapsed calls must write again; an unknown key writes nothing), using
    conn.changes() as the "did a page actually get written" signal.
  • No config/data-structure change, so no example file to sync.

Tests

  • cargo test288 passed, 0 failed (286 before; +2 = the two gates above)
  • cargo fmt --check clean
  • cargo clippy --all-targets — unchanged, only the pre-existing protocol.rs:662 warning
  • New unit tests added (the two gates)
  • A/B on the gates: reverting mmap_size to 64MB fails only
    open_does_not_enable_memory_mapping; restoring the unguarded write fails only
    touch_api_key_writes_at_most_once_per_guard_window; each leg restored byte-exact (md5 verified)

Checklist

  • Branch name follows the convention (fix/…)
  • Commit message uses Conventional Commits (fix(db): …)
  • Single purpose, minimal change — one PR for the pair the rant requires together

Not in this PR

  • Defect 3 (COUNT(*) is an O(rows) index scan per user, 963 pages / 3.8MB here) is explicitly a
    separate decision and is not touched. The three options the rant lists (30s TTL cache / exact
    per-user counter table / keyset "load more" pagination) are left for the host to choose.
  • The global single mutex amplification (a 1.9s request turning the page into 3.5s) is called out
    in the rant as an independent topic.

Acceptance note

The endpoint-level criteria (/api/transactions ≤0.3s, /api/ops/runtime ≤0.3s, /api/wallet /
/api/me / /api/dashboard ≤0.2s, and not amplified under concurrency) need this change deployed
to dev, so they are measured after the release rather than here. The projection (~70-100ms) rests on
the statement-level measurements above; they are on the live dev database, not a synthetic one.

Regression side: prod is a 2.26MB local-disk (ext4) DB, where mmap is not a slow path. Measured
read-only over 200 iterations of SELECT COUNT(*) FROM transactions:
mmap=64MB → 11ms, mmap=0 → 9ms — no regression.

Forbidden changes, deliberately avoided

No synchronous=NORMAL (corruption window under journal_mode=delete), no WAL (unsupported on NFS),
no moving the DB off the NAS, and no REINDEX/VACUUM to shuffle the physical layout: the list in the
rant's "不要做" section is respected in full.

…quest

rant 2026-09-15T10:33:34: on dev, /api/transactions 1.7-2.3s and
/api/ops/runtime 2.1s, because two independent NFS mechanisms stack.

Defect 1: db.rs open() sets PRAGMA mmap_size=64MB (v12, "readahead, the whole
file resident, remote storage read only once"). Measured on the live dev DB
(149.6 MiB, 373,322 transactions) with a read-only connection and the app's own
cache_size, same statement SELECT COUNT(*) ... WHERE user_id=2 (186,665 rows):

  mmap=64MB : 3144 / 2807 / 2894 / 1970 / 1812 ms, process read_bytes 250 MiB
  mmap=0    :   33 /   11 /   11 /   11 /   11 ms, process read_bytes  80 KiB

Reproduced twice, interleaved. The mapped pages never enter the NFS client
cache, so every query really reads the file; the pread path can use the client
cache and readahead. It is not "slow storage": the same file copied to a local
disk runs at 7ms under both settings. A size sweep (0 -> 13ms, 16MB -> 334-407ms,
64MB -> 1.7-2.2s, 128MB -> 2.2-2.8s) says every non-zero value is worse, so 0 is
the fix, not "a bigger number".

Defect 2, without which defect 1 does not pay off: every authenticated request
runs dao::touch_api_key (routes/mod.rs:135, AuthUser::from_request_parts), which
writes last_used. On a throwaway copy of the dev DB in the same NFS directory,
with a reader at mmap=0 already warm at ~10.7ms:

  unguarded write   changed=1 commit=34.2ms -> reader 1166ms  (cache dropped)
  guarded, in window changed=0 commit= 7.2ms -> reader   10.9ms
  guarded, elapsed   changed=1 commit=37.5ms -> reader 1225ms (once/min)

A write that changes the file drops the client's cached pages, so mmap=0 alone
still leaves ~1.2s behind every write. last_used only feeds "last used" in the
key list (minute resolution is plenty), so the guard drops a real write per
request to at most one per minute. SQLite does not write a page for an UPDATE
that matches no rows, which is why the guarded statement costs 7ms and leaves
the cache alone. (The gift sweep in gift.rs was re-measured under the same
harness: it writes the same value, so it does not drop the cache and is not part
of this fix - see the PR body.)

- src/db.rs: mmap_size 67108864 -> 0, with a doc comment that keeps v12's
  page-cache half (rationale, numbers, and the size sweep) and corrects the
  falsified "readahead / whole file resident" claim.
- src/dao.rs: touch_api_key gains the time guard
  (last_used IS NULL OR last_used < datetime('now','-60 seconds')), plus a doc
  comment pointing at its caller and the measurement.
- Gate tests: db::tests::open_does_not_enable_memory_mapping (mmap must stay 0,
  plus a positive control that the 64MB page cache is still set) and
  dao::tests::touch_api_key_writes_at_most_once_per_guard_window (NULL writes,
  in-window calls match 0 rows, elapsed calls write again, unknown key writes
  nothing) using conn.changes() as the "did a page get written" signal.

Not bundled: defect 3 (COUNT(*) is O(rows) per user) is a separate structural
decision the host asked to keep out of this PR.

cargo test 286 -> 288 passed. cargo fmt --check clean; clippy unchanged (only
the pre-existing protocol.rs:662 warning). The endpoint-level acceptance
(/api/transactions <=0.3s etc.) needs the fix deployed and is measured after
release, as the rant itself notes.
@argszero

Copy link
Copy Markdown
Owner Author

Self-review (as author, allow_self_merge is on for this repo; posted as a comment because GitHub
refuses --approve on one's own PR).

What I verified myself

  • cargo test on this exact tree: 288 passed / 0 failed (main was 286; +2 = the two new gates).
  • cargo fmt --check clean; cargo clippy --all-targets shows only the pre-existing protocol.rs:662
    warning.
  • Gate A/B, each leg restored byte-exact with md5 verification:
    mmap_size back to 64MB → fails only open_does_not_enable_memory_mapping;
    the unguarded write restored → fails only touch_api_key_writes_at_most_once_per_guard_window.
    The legs are disjoint, so each gate has its own teeth and neither is redundant with the other.
  • Both measurements in the description were taken on the live dev database (or a copy of it in the same
    NFS directory, removed afterwards) — not on a synthetic file.

Where I am being careful about claims

  1. The guard test asserts conn.changes() == 0. sqlite3_changes() counts rows the UPDATE matched,
    not pages written, so the test proves "the WHERE matched nothing". That implies "no page is written"
    but does not itself prove it — the file-level consequence (a warm reader staying at ~11ms) is
    evidenced by the dev measurement in the description, which CI cannot reproduce. I am not claiming the
    unit test alone establishes the NFS behavior.
  2. last_used is only display data (minute resolution is enough), which is what makes the guard
    acceptable. If a future feature needs second-level "last used", the guard window has to be revisited.
  3. The change is a pragmatic trade against a specific measured pathology: on this NFS, mmap is a slow
    path. On a local-disk deployment it is simply neutral (prod: 11ms vs 9ms per 200 iterations), so the
    change carries no cost for the other deployment. I deliberately did not try to make the setting
    conditional on storage type — that would add a configuration knob for a problem the raw pragma can't
    detect reliably, and 0 is at worst neutral everywhere measured.
  4. Endpoint-level acceptance is not claimed here: it requires this to be deployed to dev. The rant
    itself notes the projection comes from statement-level A/B, to be re-measured after landing.

Scope discipline

Defect 3 (COUNT(*) O(rows)) and the global-mutex amplification are both left out, as the rant
requires. Nothing here prepares them: this PR only removes two costs the endpoints pay unconditionally.

@argszero
argszero merged commit a28ad3e into main Sep 15, 2026
1 check passed
@argszero
argszero deleted the fix/nfs-mmap-and-per-request-write branch September 15, 2026 03:06
@argszero argszero mentioned this pull request Sep 15, 2026
10 tasks
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