7392bb63 - Project asset prices in SQL instead of loading whole snapshots - #4464
Conversation
…apshots LedgerMarkService.preload loaded complete log rows and parsed each message in Node, only to keep three values per asset: created, assetId and priceChf. A snapshot carries 200 assets in an average 42 KB JSON document. Measured on the real two-day preload window in production: 930 rows and 38 MB transferred, of which 7.3 MB is the data actually used. The JSON.parse over those 38 MB runs on the event loop and is a direct cause of the API container sitting at a saturated core, which slows every request. The new repository method projects the prices in SQL via LATERAL jsonb_each and returns flat rows. The LIMIT and the keyset cursor stay on the log rows, not on the expanded result, so pagination semantics are unchanged — the cursor therefore carries the originating log id. getLatestMarks keeps using getFinancialLogs: it runs on the dailySample path, where the row count is already small and the added complexity would not pay for itself. Mark map contents, ordering and the handling of missing or non-finite prices are unchanged.
…dation Review follow-ups on the SQL price projection. A log row whose assets yield no usable price used to vanish from the projection entirely. Overflow detection and keyset pagination count log rows, so a vanished row made them undercount: preload could miss that more data exists, and paginate could stop early — a silent loss of marks. The projection now uses LEFT JOIN LATERAL, so every read log row comes back even when it carries no usable price, and the counting is correct again. This also removes a related false end-of-data path in the stale-cursor guard. Price conversion now matches the previous Number.isFinite semantics: a price is only taken when the JSON value really is a number, so a string "1.25" is skipped as before rather than silently accepted by the float cast. Non-numeric asset keys no longer abort the whole query; they yield null and are skipped. Malformed message JSON now fails loudly instead of being swallowed — deliberate, and documented on the method. buildMarkMap keeps Number.isFinite as a second line of defence. The repository already nulls NaN and Infinity, but a NaN mark would corrupt a valuation silently, so that must not rest on a single guard. The test fake mirrors the SQL semantics instead of masking it, and three tests cover the cases: a log row without usable prices between two valid ones, a string price, and a non-numeric asset key.
|
Ready after one review round, four findings, all addressed. Two blocking, same root cause. A log row whose assets yielded no usable price vanished from the projection entirely — but overflow detection and keyset pagination count log rows. A vanished row made them undercount, so Verified in production before fixing, to judge the risk honestly: all 3'000 sampled snapshots carry exactly 200 assets with 200 usable prices, and all 60'000 asset keys are numeric with no price stored as a string. So the failure mode is currently unreachable — but no database invariant enforces that, and silent data loss in accounting code should not depend on a writer's habits. Two major, semantics. The The test fake mirrored the old tolerant JS behaviour and therefore kept a test green that production would fail. It now mirrors the SQL semantics instead. One point I added beyond the review: Verification: 963 tests across 40 suites green, lint and type-check clean, CI green (12/12), both commits signed. The blocking case has a dedicated regression test, and its sharpness was confirmed by reverting to the old row-dropping fake — it then fails with a lost mark (expected 3, received 1). |
1 participant
Third and largest step toward the goal that every API call completes in under 1 second. Currently only 84–91 % do, depending on the window.
Problem
LedgerMarkService.preload()loaded completelogrows and parsed eachmessagein Node — only to keep three values per asset:created,assetIdandpriceChf. A snapshot carries 200 assets in an average 42 KB JSON document.Measured on the real two-day preload window in production:
So 81 % of the transfer is waste. Worse, the
JSON.parseover those 38 MB runs on the event loop — and that is a direct cause of the API container sitting at a saturated core, which slows down every request in the system, not just ledger work.Context: the query itself executes server-side in 86 ms but sits at 7,3 s in
pg_stat_activity. The gap was never execution — it was transfer and deserialisation.Change
New repository method projecting the prices in SQL via
LATERAL jsonb_each, returning flat rows instead of documents:LIMITand the keyset cursor stay on the log rows (inner subquery), not on the expanded result — otherwise pagination semantics would change. The cursor therefore carries the originatinglogId, and the existing microsecond-safe cursor resolution plus the fail-loud stale-cursor guard are reused unchanged.buildMarkMap()now consumes the projection directly;parseAssets()is no longer needed on this path. Mark contents, ordering and the handling of missing or non-finite prices are unchanged — that is the invariant this PR must not break, since ledger valuation depends on it.getLatestMarks()deliberately keeps usinggetFinancialLogs: it runs on thedailySamplepath where the row count is already small, so the added complexity would not pay for itself.Verification
960 tests across 40 suites green, lint and type-check clean. Three new tests cover the invariant directly: the projection produces the same mark map as the former full-document path, non-finite prices are still skipped rather than turned into a zero mark, and pagination continues to cursor over log ids without losing a row when several result rows share one log id.
Scope
This is the transfer and CPU problem. Disk I/O and the sort step were addressed separately by the index in #4457.