internal/db2/history: rewrite /assets pagination as bounded CTE merge query - #210
internal/db2/history: rewrite /assets pagination as bounded CTE merge query#210hexdigest wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Reworks /assets pagination to merge bounded, index-oriented query branches instead of using a non-sargable full outer join.
Changes:
- Splits asset retrieval into
exp_asset_statsand contract-only CTEs. - Merges, orders, and limits both result sets.
- Documents the performance improvement.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
internal/db2/history/asset_stats.go |
Implements bounded CTE pagination. |
CHANGELOG.md |
Records the /assets performance improvement. |
Comments suppressed due to low confidence (1)
internal/db2/history/asset_stats.go:517
- This row-valued
ORDER BYlikewise cannot be satisfied byasset_contracts' multi-column primary-key ordering, so broad requests sort the full contract-only candidate set beforeLIMIT. Use separate ordering expressions so this branch can stop after the firstpage.Limitindex-ordered rows.
OrderBy("(asset_contracts.asset_code, asset_contracts.asset_issuer) " + orderBy).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi all, I know Horizon is being decommissioned, but we’re currently seeing production latency issues from this class of /assets queries. In the worst cases, latency can spike to around 10 seconds, which is not acceptable for some of our clients. This change is intended to address that specific query shape by restoring index-backed pagination and filtering, and the before/after EXPLAIN ANALYZE results show a substantial improvement for the affected requests. |
tamirms
left a comment
There was a problem hiding this comment.
Nice work — the decomposition is the canonical fix for pagination over a FULL OUTER JOIN, and I verified the semantics are preserved: the NULL→zero-JSON swap matches what the Scan implementations already normalize, placeholder rebinding through SelectRaw is correct, and the existing TestGetAssetStatsFiltersAndCursor suite (which includes a contract-only asset) covers both branches, filters, cursors, and both orders.
One ask before merge: a test addition. The checklist marks tests as added but the diff contains none. Existing coverage is genuinely strong, with one gap: no case filters a contract-only asset by code (BTC only appears unfiltered or issuer-filtered). An assetCode: "BTC" case in TestGetAssetStatsFiltersAndCursor would exercise the NOT EXISTS branch under exactly the filter shape this PR optimizes.
| `'{"authorized":0,"authorized_to_maintain_liabilities":0,"claimable_balances":0,"liquidity_pools":0,"unauthorized":0}'::jsonb as accounts`, | ||
| `'{"authorized":"0","authorized_to_maintain_liabilities":"0","claimable_balances":"0","liquidity_pools":"0","unauthorized":"0"}'::jsonb as balances`, |
There was a problem hiding this comment.
Consider selecting typed NULLs here instead of hand-written zero JSON:
| `'{"authorized":0,"authorized_to_maintain_liabilities":0,"claimable_balances":0,"liquidity_pools":0,"unauthorized":0}'::jsonb as accounts`, | |
| `'{"authorized":"0","authorized_to_maintain_liabilities":"0","claimable_balances":"0","liquidity_pools":"0","unauthorized":"0"}'::jsonb as balances`, | |
| `NULL::jsonb as accounts`, | |
| `NULL::jsonb as balances`, |
The old FULL OUTER JOIN produced NULL accounts/balances for contract-only rows, and ExpAssetStatAccounts.Scan / ExpAssetStatBalances.Scan already normalize NULL to exactly these zero values (0 ints, "0" strings). Relying on that gives byte-for-byte parity with the previous behavior and removes two hard-coded copies of the struct field sets, which would silently drift if a field is ever added to ExpAssetStatAccounts/ExpAssetStatBalances.
| if len(results) == 0 { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
This block is dead code — results starts nil and sqlx only appends per scanned row, so zero rows already yields nil (clearSliceIfPossible preserves nil-ness too). That's why the existing expected: nil cases in TestGetAssetStatsFiltersAndCursor passed before this change, on the same SelectRaw path.
| if len(results) == 0 { | |
| return nil, nil | |
| } |
PR Checklist
PR Structure
Thoroughness
Release planning
I've reviewed the changes in this PR and if I consider them worthwhile for being mentioned on release notes then I have updated the relevant CHANGELOG.md within the component folder structure. For example, if I changed horizon, then I updated (CHANGELOG.md (CHANGELOG.md). I add a new line item describing the change and reference to this PR. If I don't update a CHANGELOG, I acknowledge this PR's change may not be mentioned in future release notes.
I've decided if this PR requires a new major/minor version according to semver (https://semver.org/), or if it's mainly a patch change. The PR is targeted at the next release branch if it's not a patch change.
What
Rewrite internal/db2/history /assets pagination to use a bounded CTE merge query instead of paginating across a FULL OUTER JOIN with COALESCE(...).
The new query:
This preserves the existing response semantics while avoiding full-table scan and sort behavior.
Why
The previous /assets query paginated over a FULL OUTER JOIN and used COALESCE(...) in both filtering and ordering. That made the query non-sargable, so Postgres could not use the existing indexes effectively.
For the request:
/assets?asset_code=USDC&asset_issuer=GA5ZSEJYB37JRC5AVCIA5MOP4RHTM335X2KGX3IHOJAPP5RE34K4KZVN&limit=1the
EXPLAIN ANALYZEresults were:Before
After
This reduces execution time for that request from 130.288 ms to 2.696 ms while changing the plan from full scans plus a full join to bounded index-backed branches.
Known limitations