7392bb63 - Index the ledger content-change scan and project financial-log fields in SQL - #4480
Merged
TaprootFreak merged 9 commits intoJul 30, 2026
Conversation
… fields in SQL Two independent sources of database load on dfxprd, both measured in production. 1. The nine ledger consumers each run a (updated, id) keyset content-change scan every minute, but none of their source tables had an index on `updated`. EXPLAIN (ANALYZE, BUFFERS) for trading_order showed a Parallel Seq Scan reading ~494 MB from disk and filtering 5,418,069 rows to return 4, at 151.765 ms per call. Add composite (updated, id) indexes on all nine source tables. 2. GET /v1/dashboard/financial/log selected every matching row including the ~42 KB `message` JSON: 1,316 MB of message payload across the 31,925 matching rows, requested twice per minute, with a 6.6 s median and 29.8 s maximum query time. The endpoint needs only four `balancesTotal` scalars, one BTC price and the `balancesByFinancialType` sub-tree — `assets` (1,346 MB) and `tradings` (43 MB) were never read. Project just those sub-trees in SQL. The response is unchanged for the same underlying data. Malformed `message` JSON now aborts the query instead of silently dropping the row: for a financial dashboard a visible failure is preferable to an unnoticed gap in the series. Verified that all 31,925 matching rows currently hold valid JSON.
Replace the nine self-invented index names in AddLedgerContentChangeScanIndexes1785460000000 with the deterministic names TypeORM's DefaultNamingStrategy would generate itself, per the "No custom naming for TypeORM indexes" rule. Also documents the derivation in the migration's docstring so the names can be recomputed.
- Restore the falsy btcAssetId check (0/undefined/null all take the no-BTC-asset path) that extractBtcPrice used to have, instead of only excluding undefined. - Guard all five projected numbers (totalBalanceChf, plusBalanceChf, minusBalanceChf, btcPriceChf, fxPnlChf) with a jsonb_typeof check so one bad value nulls only that field instead of aborting the whole query, and align the fxPnlChf guard with the same pattern used by the neighboring priceChf projection. - Remove the now-unused extractBtcPrice helper. - Add coverage for the ORDER BY direction, the exact $N parameter position per SQL condition, and the projection's edge cases (btcAssetId 0/undefined, missing/null balancesTotal fields), and drop assertions that only read back their own test fixture.
…gration The previous note claimed a CREATE INDEX SHARE lock is held only for the duration of that one index build, and justified the write-blocking risk for trading_order on that basis. That was wrong. All nine CREATE INDEX statements run inside a single transaction: TypeORM's migrationsTransactionMode defaults to "all" (DataSource.js:263-265) and config.ts never overrides it, and MigrationExecutor.js:206 opens one transaction for all pending migrations. PostgreSQL releases locks at COMMIT, not per statement. The write-blocking window for trading_order is therefore the sum of all nine builds, and once the transaction commits all nine tables are simultaneously write-blocked -- including bank_tx, buy_crypto, crypto_input and payout_order. Splitting this across several migration files would not change that. Also drop the redundant SET LOCAL lock_timeout calls. SET LOCAL is scoped to the transaction, so 18 of the 19 had no effect and implied a per-statement guard that does not exist; one call in up() and one in down() remain.
totalBalanceChf/plusBalanceChf/minusBalanceChf now stay null when the SQL projection nulls them, matching the fxPnlChf handling already in place; the `?? 0` default remains solely at the mapSummaryToEntry call site. Also drop the Number() coercion in the balancesByType loop so a genuinely missing key passes through unconverted instead of becoming NaN/null, and fail loud with the log id and type key when a balancesByFinancialType entry is not an object instead of throwing an unhandled TypeError. Narrows the "byte-for-byte identical" docstring claim to its actual scope and documents down()'s ACCESS EXCLUSIVE lock behaviour and the per-statement scope of lock_timeout in the index migration.
FinancialLogSummary.balancesByType and FinancialLogEntryDto.balancesByType promised plain `number` fields, but a real production row has an entry missing one of the two keys, which the repository already passes through as `undefined` (proven by an existing test). Mark both fields optional and narrow the `as` cast accordingly instead of forcing a promise the code doesn't keep. Also drop the now-unreachable JSON-string branch when parsing balancesByFinancialType: pg-types always returns jsonb columns already parsed (OID 3802), so the string path had zero coverage and no path to it. Most importantly, revert the typeof/null throw added for a prior finding: it was too broad. Before that guard, a primitive balancesByFinancialType entry (number/string/boolean) never threw at all - JS auto-boxes primitives for property access, so e.g. `(1).plusBalanceChf` is simply `undefined`. Only `null` used to throw, and only inside the old mapper's per-row try/catch, silently dropping just that one row. The throw turned three previously-harmless cases into a 500 for the whole request, and widened null's blast radius from "one row" to "everything". Optional chaining covers all of it in one line: primitives resolve to undefined exactly as before, and null now keeps the row (with an empty balancesByType entry) instead of either silently vanishing or failing the whole request.
Two comments claimed more than the code delivers. mapSummaryToEntry still promised a byte-identical response. That stopped being true on purpose: a `balancesByFinancialType` entry holding `null` used to make the previous mapLogToEntry drop the entire log line via its per-row try/catch, whereas the row is now kept with an empty entry. A silent gap in a financial curve is worse than an empty partial entry. The same comment also claimed SQL "fails loud" without qualification, while individual scalar values are in fact tolerated by the jsonb_typeof guards. The balancesByType loop now records why it deliberately has no per-property typeof guard: the case is unproven in production (287,989 entries with both values numeric, one with plusBalanceChf missing, none with a string, boolean or null value), a guard would break response equivalence with the old mapLogToEntry which passed such values through unchanged, and unlike the five scalar fields there is no ::float8 cast here that could abort the query.
The signature promises `plusBalanceChf?: number`, but a balancesByFinancialType entry whose property itself holds a string, boolean or null was passed through unchanged, so the endpoint could return values the DTO does not allow. The previous mapLogToEntry had the same hole; this closes it instead of carrying it forward. Only real numbers are kept now; every other value becomes undefined. On the current production data this changes nothing (287,989 entries with both values numeric, one with plusBalanceChf missing, none holding a string, boolean or null), so it exists to protect the contract for future data. A missing key still yields undefined, and a non-object entry still does not throw. This applies the same hardening the five scalar fields already get via jsonb_typeof in SQL, in TypeScript because balancesByFinancialType is passed through as a raw JSON object.
TaprootFreak
marked this pull request as ready for review
July 30, 2026 07:17
TaprootFreak
deleted the
perf/ledger-scan-index-and-financial-log-projection
branch
July 30, 2026 07:18
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.
Why
All database-backed endpoints on dfxprd were 6–30x slower than usual, while endpoints that touch no database stayed unchanged. I traced this to two independent sources of load and measured both in production.
1. The per-minute content-change scan has no index to use.
The nine ledger consumers each run
@DfxCron(EVERY_MINUTE)and callrunContentChangeScan, which issues a(updated, id)keyset scan:None of the nine source tables had an index on
updated— I checked 17 tables, none had one. ProductionEXPLAIN (ANALYZE, BUFFERS)fortrading_order(921 MB, the largest):A 45-second delta measurement attributed 4,750 MB of disk reads and 32.5 million rows read to
trading_orderalone.2.
GET /v1/dashboard/financial/logtransfers the whole message JSON.The endpoint is called twice per minute (121 calls in 2 hours) and the query that runs is the unsampled full-column path.
How that is established — the caller's parameters are not visible in the access log, because morgan here logs the path without the query string (verified: not one log line in a full hour contains a
?, across 5–7 requests/s). The evidence is the SQL actually observed inpg_stat_activity:Two things follow from its shape: there is no
id IN (SELECT MAX(id) ... GROUP BY CAST(created AS DATE)), sodailySamplearrives asfalse(the controller maps a missing parameter totrueviadailySample !== 'false', so it cannot be absent); andcreated >= $5is present, sofromis supplied. There is no upper bound and noLIMIT.Measured payload of the full matching set (the upper bound of what this path can transfer; the actual per-call volume depends on the caller's
from):Production query time: 6.6 s median, 29.8 s maximum. In 64 of 168
pg_stat_activitysamples this exact query was running, frequently in wait eventClient:ClientWrite.Together these produce roughly 105 MB/s of sustained reads against 5–7 actual requests/s. The Postgres container is capped at 4 GB and sits at 99.99% of that limit with ~62 memory reclaim events/s and
pgsteal/pgscanat ~100%, so no working set survives in cache and evenGET /v1/asset(18 -> 187 ms) has to hit disk. That container limit lives in the infrastructure repo and is deliberately out of scope here.Evidence that this is not event-loop saturation: CORS preflights, which go through Express but touch no database, stayed at 0.1 ms in both a healthy and a degraded window (factor 1.2–1.8), while every database-backed endpoint degraded by 6–30x.
What this changes
Part A — new migration adding composite
(updated, id)indexes on all nine consumer source tables:trading_order,crypto_input,bank_tx,buy_crypto,exchange_tx,payout_order,buy_fiat,liquidity_management_order,liquidity_order.Column order is intentional: the query orders by
updated, id, so an index onupdatedalone would still need an explicit Sort whenever rows share anupdatedvalue — the same reasoning asAddFinancialLogQueryIndex1785400000000for(created, id).CREATE INDEX CONCURRENTLYis not used because migrations here run transactionally and boot-blockingly (migrationsRun, gated bySQL_MIGRATE);CONCURRENTLYis not permitted inside a transaction.On lock duration, stated precisely: a plain
CREATE INDEXholds a SHARE lock for the entire build, so reads continue but writes to that one table block until it finishes. I measured the scan+sort of(updated, id)on the largest table at 1159 ms, but that figure is awork_mem-bound external merge sort spilling ~116 MB to disk — not the index build itself, which sorts inmaintenance_work_memand should be faster, plus the cost of writing ~150 MB of index pages. That points at a low single-digit-second range but is explicitly not a measured upper bound. Judged acceptable becausetrading_ordertakes roughly one write every 20 seconds (~4,350 rows/day) and its writers retry.Part B — new repository method
getFinancialLogSummariesprojects only the three needed sub-trees in SQL (balancesTotalscalars, the optional BTCpriceChf, and thebalancesByFinancialTypesub-object).assetsandtradingsare never selected.DashboardFinancialService.getFinancialLogconsumes the projection;getBtcCoin()now resolves before the query because the BTC price is projected in SQL, costing one sequential roundtrip that is negligible against the eliminated transfer.The response is unchanged for the same underlying data.
fxPnlChfdeliberately staysnullin the projection so the existing?? 0default remains at the mapping call site rather than being introduced in the data layer.Behaviour change, stated explicitly
Malformed
messageJSON now aborts the query instead of silently dropping that one row:message::jsonbfails loud, whereas the previous per-rowtry/catchreturnedundefinedand the row was filtered out. For a financial dashboard a visible failure is preferable to an unnoticed gap in the series.I verified against production that this is not a latent risk today, in two separate steps, because JSON validity alone would not have been sufficient evidence:
message::jsonbover all matching rows succeeds; there is no row holding invalid JSON.::float8cast. Measured type distribution over 31,956 rows:No string or boolean values exist. For the four rows with an absent key or JSON
null,->>yields SQL NULL, which::float8casts to NULL rather than failing — confirmed by casting all 31,956 rows in one query without error. Those rows then map to0, exactly as the previousfinanceLog.balancesTotal?.totalBalanceChf ?? 0did, so the response is unchanged for them too.What this does not cover: a future writer emitting a string into one of these fields would abort the whole query rather than degrade one row. Unlike the neighbouring
getFinancialLogAssetPrices, this projection does not guard the cast withjsonb_typeof(...) = 'number'. That is a robustness gap, not a present defect, and is tracked as a follow-up.What this PR does not promise
Both inefficiencies above are measured facts about the code: reading ~494 MB per call to return four rows, and shipping a message payload of which the endpoint reads three small sub-trees. Removing them is worth doing on its own terms.
It would be wrong, though, to present this as a guaranteed latency figure. Observed p50 of the access-log latency on dfxprd over the same day, at essentially constant request volume (5.5–6.4 req/s throughout):
During the 2.2 ms stretch the content-change scans were still running at the same rate (a 45 s delta attributed 2,788 MB of disk reads to
trading_orderalone — more than during a 15 ms window earlier), andGET /v1/dashboard/financial/logwas still being called 60 times per hour. So the relationship between this load and end-user latency is not monotonic: the same load coexists with p50 values two orders of magnitude apart, evidently gated by cache state rather than by request volume. There is also an unexplained pattern where restarting the API container alone shifts latency for hours.In other words: this PR removes two well-documented sources of avoidable database load, and the load reduction itself is straightforwardly verifiable after deploy (index usage via
pg_stat_user_indexes, disappearance of the seq scans, transferred payload size). Whether p50 lands at 2 ms or 200 ms afterwards depends on factors this change does not control — notably the 4 GB memory cap on the Postgres container, which is infrastructure and deliberately out of scope here.Verification
All CI gates mirrored locally:
format:check,lint,type-checkand the test suites for the touched areas (125 tests, 5 suites, green).Test sharpness is backed by a mutation run rather than asserted. Baseline green, then five deliberate defects injected one at a time:
M5 is a genuine gap: the
ORDER BY created ASC, id ASCguarantee is untested, and it carries both the chart's chronological order and theafterkeyset pagination. A test for it is still to be added.The migration SQL was not executed against production.