7392bb63 - Index the financial-log query columns - #4457
Merged
Conversation
The log table is the largest in the database at 1353 MB (527339 rows) and carried only its primary key. The minute-interval LedgerMarkService query filters on system, subsystem, severity and valid with a created range, then orders by created — which forced a Parallel Seq Scan plus an explicit Sort, reading ~674 MB from disk per call and discarding 175629 rows per worker at the filter. The composite index puts the four equality predicates first and the range column last, so Postgres can serve both the filter and the ordering from it. This addresses disk load and the sort step only. The transferred payload — up to 5001 rows carrying a 5.8 KB message column, roughly 47 MB per call — is unchanged and remains the dominant cost on the Node side.
Review follow-ups. The query orders by (created, id), so an index ending at created still needs a sort step whenever rows share a created value. id is now the sixth column. The lock comment claimed a short SHARE lock. It is held for the entire build, and lock_timeout caps only the wait to acquire it, not the hold. The build duration was not measured against production, so no upper bound is claimed. down() now schema-qualifies the index like every existing migration does.
TaprootFreak
marked this pull request as ready for review
July 29, 2026 15:06
Collaborator
Author
|
Ready after one review round, three findings, all addressed:
CI green on the final commit (13/13, including Migration immutability), both commits signed. |
This was referenced Jul 29, 2026
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.
Follow-up to #4446.
Goal
Every API call should complete in under 1 second. Measured against production right now, we are far from it:
Even p90 is over the target. This PR is one step, not the answer.
What this changes
The
logtable is the largest in the database at 1353 MB / 527'339 rows and carried only its primary key. The minute-intervalLedgerMarkServicequery filters onsystem,subsystem,severity,validplus acreatedrange and orders bycreated, id.EXPLAIN (ANALYZE, BUFFERS)in production:The composite index
(system, subsystem, severity, valid, created, id)puts the four equality predicates first, then both ORDER BY columns in query order.idis included deliberately: an index ending atcreatedwould still require a sort whenever rows share acreatedvalue, which happens regularly at ~2 writes per minute.What this does NOT fix
Disk I/O and the sort step. The query still returns up to 5'001 rows carrying an average 5.8 KB
messagecolumn — roughly 47 MB transferred per call — which is the dominant remaining cost and keeps the Node event loop busy. Measured: the query executes server-side in 86 ms but sits at 7.3 s inpg_stat_activity; the difference is transfer and deserialisation, not execution.Closing that gap means aggregating the needed prices in SQL instead of shipping raw JSON, which changes ledger logic and belongs in its own PR.
Migration notes
CONCURRENTLY— migrations here run transactionally and boot-blocking (migrationsRun, gated bySQL_MIGRATE), andCREATE INDEX CONCURRENTLYis not permitted inside a transaction.CREATE INDEXholds a SHARE lock for the entire build, not briefly. Reads continue; writes tologblock for the duration.SET LOCAL lock_timeoutcaps only the wait to acquire the lock, not how long it is held. The build duration has not been measured against production data, so no upper bound is claimed. Judged acceptable becauselogtakes ~2'900 rows/day and those writes are retried by their jobs rather than lost.IF NOT EXISTS— the migration should fail loudly if the index unexpectedly exists rather than continue silently.Review
One round, three findings, all addressed:
idadded as the sixth column (the original claim of eliminating the sort was not fully accurate), the misleading "short SHARE lock" wording corrected, anddown()now schema-qualifies the index like every existing migration.