internal/db2: Reduce temp-file I/O on account-filtered /trades queries#181
Open
tamirms wants to merge 2 commits into
Open
internal/db2: Reduce temp-file I/O on account-filtered /trades queries#181tamirms wants to merge 2 commits into
tamirms wants to merge 2 commits into
Conversation
Change the subquery UNION in createTradesSQL to UNION ALL and push the outer LIMIT into each subquery. The two branches are disjoint by protocol invariant (an account cannot match its own offer, a trade matches two distinct offers, and an LP trade has the pool on exactly one side), so UNION ALL is semantically equivalent and skips the expensive dedup sort that was generating roughly 100 GB of temp writes per execution at pubnet scale. Add composite indexes on (base_account_id, history_operation_id, "order") and (counter_account_id, ...) so each subquery can satisfy both the WHERE filter and ORDER BY from an index range scan, giving a density-independent spill-free plan. The existing single-column indexes are retained until follow-up observation confirms the composites cover all usage. Validated on staging (324M row history_trades): execution time drops from 174 s (baseline) to 4.6 ms / 6.75 ms / 3.4 ms across hot, mid-tier, and sparse account classes; zero temp-file I/O. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes account/offer/liquidity-pool filtered /trades queries by rewriting the generated SQL to avoid expensive dedup sorts and by adding supporting composite indexes, targeting a major source of PostgreSQL temp-file I/O and replica lag on pubnet-scale datasets.
Changes:
- Update
createTradesSQLto useUNION ALL(instead ofUNION) for base/counter filter branches and apply per-branchLIMITbefore the final order/limit. - Add composite indexes on
(base_account_id, history_operation_id, "order")and(counter_account_id, history_operation_id, "order")usingCREATE INDEX CONCURRENTLY. - Add a unit test to prevent regressions back to
UNION, and update embedded migration bindata + changelog.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/db2/schema/migrations/71_trades_account_composite_indexes.sql | Adds concurrent composite indexes to support efficient account-filtered trade pagination. |
| internal/db2/schema/bindata.go | Regenerates embedded schema assets to include migration 71. |
| internal/db2/history/trade_test.go | Adds a regression test asserting account/offer/pool variants use UNION ALL. |
| internal/db2/history/trade.go | Rewrites unioned account/offer/pool query generation to UNION ALL and pushes LIMIT into each branch. |
| CHANGELOG.md | Documents the query optimization and the new migration/indexes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The previous commit regenerated bindata.go with a go-bindata version that
emits explicit &bintree{x, y} struct literals. gofmt -s canonicalizes
those to the implicit {x, y} form, which is what the CHECK CI job
expects. No semantic change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tamirms
changed the title
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.
Summary
createTradesSQLto useUNION ALL+ per-branchLIMITon the account/offer/pool filter variants. The subqueries are disjoint by protocol invariant (self-matching prevention + single-sided LP trades), soUNION ALLis semantically equivalent and skips an expensive dedup sort.htrd_by_{base,counter}_account_op_orderso each subquery satisfies WHERE + ORDER BY from an index range scan.Impact (measured on staging, 324M rows)
Correctness: analytical disjointness proof
Empirically verified on staging (three
SELECT count(*)scans ofhistory_tradesall returned 0), and analytically confirmed againstinternal/ingest/processors/trades_processor.go:base_account_id = counter_account_id: impossible. Orderbook trades have seller != buyer (stellar-core self-match prevention). LP trades havebase_account_id = NULL(the counterparty is the pool).base_offer_id = counter_offer_id: impossible. Orderbook trades match two distinct offers; the TOID-encoded synthetic counter offer id is in a disjoint encoding domain from raw offer ids. LP trades havebase_offer_id = NULL.base_liquidity_pool_id = counter_liquidity_pool_id: impossible. Orderbook trades have both NULL. LP trades have exactly one side set to the pool id.The existing
htrd_by_base_account/htrd_by_counter_accountsingle-column indexes are retained; a follow-up PR will drop them after confirming the composites cover their workload.🤖 Generated with Claude Code