Skip to content

split paginated trades query in 2 - #4954

Merged
MartinquaXD merged 14 commits into
mainfrom
reduce-load-from-trades-queries
Sep 23, 2026
Merged

MartinquaXD merged 14 commits into
mainfrom
reduce-load-from-trades-queries

Conversation

@MartinquaXD

@MartinquaXD MartinquaXD commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Description

The /trades endpoint allows to pass 2 different filters (by order_uid and by owner). Those are currently mutually exclusive but are handled by the same underlying postgres query. That means the query has to be generic enough to serve both use cases which prevents some optimizations and makes the query quite complicated to achieve reasonable performance.

Changes

Model the query filter as an enum and split the postgres query into 2 dedicated queries. Especially the pathological case that gets triggered a lot recently (one order with MANY partial fills) can be optimized more with a dedicated query.

Also added a new index which also turns the last sub-query of the owner based query into an index only scan.
The index was already created on all the prod DBs (they were super fast to create anyway) so I decided to not use CONCURRENTLY when creating the index. That way was can create the new index and drop the old one in a single transaction.

How to test

existing postgres tests still pass

Synthetic benchmark on cached queries

  ┌─────────────────────────┬────────────┬───────────────────────────┬────────────────┐
  │          Query          │ First page │ Last page (offset=17,944) │ Scaling factor │
  ├─────────────────────────┼────────────┼───────────────────────────┼────────────────┤
  │ ORIGINAL (owner filter) │       16.3 │                     121.3 │           7.4× │
  ├─────────────────────────┼────────────┼───────────────────────────┼────────────────┤
  │ by_owner                │       13.3 │                      48.5 │           3.6× │
  └─────────────────────────┴────────────┴───────────────────────────┴────────────────┘

  Scenario 2: EOA-like heavy owner (0x6aba... — 26,285 trades, all branch 1)

  ┌─────────────────────────┬────────────┬───────────────────────────┬────────────────┐
  │          Query          │ First page │ Last page (offset=25,285) │ Scaling factor │
  ├─────────────────────────┼────────────┼───────────────────────────┼────────────────┤
  │ ORIGINAL (owner filter) │       16.7 │                     138.4 │           8.3× │
  ├─────────────────────────┼────────────┼───────────────────────────┼────────────────┤
  │ by_owner                │       12.9 │                      59.4 │           4.6× │
  └─────────────────────────┴────────────┴───────────────────────────┴────────────────┘

  Scenario 3: Single UID with many partial fills (0xb402c7c9... — 9,680 fills)

  ┌───────────────────────┬────────────┬──────────────────────────┬────────────────┐
  │         Query         │ First page │ Last page (offset=8,680) │ Scaling factor │
  ├───────────────────────┼────────────┼──────────────────────────┼────────────────┤
  │ ORIGINAL (uid filter) │       10.9 │                     48.8 │           4.5× │
  ├───────────────────────┼────────────┼──────────────────────────┼────────────────┤
  │ by_order_uid          │        6.5 │                     10.5 │           1.6× │
  └───────────────────────┴────────────┴──────────────────────────┴────────────────┘

Test on prod. I created a scrip that exercised the same pathological behavior as the recent BNB spammer (1 order with thousands of trades with a huge offset and limit). The first diagram shows the number of problematic requests that were handled by the system. And the second diagram shows how much the DB was stressed during the time.
I wrote a small script to spam our orderbook myself and as we can see despite sending roughly twice the number of requests as the real spammer the DB was under significantly less load with the 2 specialized queries.

Screenshot 2026-09-22 at 12 03 49 Screenshot 2026-09-22 at 12 03 57

@MartinquaXD
MartinquaXD requested a review from a team as a code owner September 22, 2026 08:47
@claude

claude Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MartinquaXD's task in 4m 25s —— View job


PR Review: split paginated trades query in 2

I reviewed the SQL rewrite and the orderbook wiring. Overall this is a clean, well-documented change and the semantics of the old unified query are faithfully preserved:

  • Correctness ✅ — The three owner branches are disjoint on (block_number, log_index), the page CTE dedups with DISTINCT, and hydration joins on the trades PRIMARY KEY (block_number, log_index) so there's no fan-out. The LIMIT $2 + $3 per branch + outer LIMIT/OFFSET reproduces the original pagination. The EXISTS guard in trades_by_order_uid correctly filters orphan trades for both orders and jit_orders.
  • Owner semantics preserved ✅ — onchain orders still return orders.owner (placement contract) via COALESCE, matching the pre-PR behavior.
  • Wiring ✅ — the TradeFilter enum makes the "exactly one filter" invariant type-level, dispatch in database/trades.rs is correct, and awaiting the Instrumented future directly (dropping .into_inner()) keeps the span. Default was removed cleanly with no remaining users.

I left two non-blocking inline comments:

  1. A likely-redundant JOIN orders in the onchain-sender branch (small per-row cost in a query this PR is optimizing).
  2. A test-coverage gap: the JIT-owner branch / JIT hydration path has no postgres test.

Note: I couldn't run cargo check/tests in this environment (command not permitted), so the compile/DB-test verification relies on the author's local runs and CI.
· reduce-load-from-trades-queries

Comment thread crates/database/src/trades.rs
Comment thread crates/database/src/trades.rs
@MartinquaXD
MartinquaXD marked this pull request as draft September 22, 2026 09:41
@MartinquaXD
MartinquaXD marked this pull request as ready for review September 22, 2026 12:09
@claude

claude Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MartinquaXD's task in 2m 52s —— View job


PR Review: split paginated trades query in 2

Nice change — splitting the unified query into trades_by_order_uid / trades_by_owner reads clearly, the TradeFilter enum makes the "exactly one filter" invariant type-level, and the wiring in database/trades.rs and orderbook.rs is faithful to the old behaviour. Pagination semantics (LIMIT limit+offset per branch → outer LIMIT/OFFSET), the owner-across-three-tables split, and the settlement/hydration laterals all look correct.

I left one inline finding:

  • Branch 3 (trades_by_owner) can drop rows on deep pages. jit_orders.uid is not unique (PK is (block_number, log_index)), so the jit_uids CTE can emit duplicate uids, which multiply matching trades before the per-branch LIMIT offset+limit. The DISTINCT in the page CTE runs after that truncating LIMIT, so it can't recover the pushed-out rows. Same as the old query's behaviour (not a regression), but cheaply fixable with SELECT DISTINCT uid … — details + repro in the inline comment.

The two points from the earlier automated review still stand and I won't restate them: the likely-redundant JOIN orders in the onchain branch, and the missing postgres test for the JIT owner/hydration path (a test would also guard the duplicate-uid case above).

Note: I couldn't run cargo check/DB tests in this environment, so compile/test verification relies on CI and the author's local runs.
· reduce-load-from-trades-queries

Comment thread crates/database/src/trades.rs Outdated
@github-actions

Copy link
Copy Markdown

Reminder: Please update the DB Readme and comment whether migrations are reversible (include rollback scripts if applicable).

  • If creating new tables, update the tables list.
  • When adding a new index, consider using CREATE INDEX CONCURRENTLY for tables involved in the critical execution path.
  • For breaking changes, remember that during rollout k8s starts the new autopilot, runs the Flyway migration, and only then shuts down the old pod. That overlap means the previous version can still be processing requests on the migrated schema, so make it compatible first and ship the breaking DB change in the following release.

Caused by:

@AryanGodara AryanGodara left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a few comments. Looks good overall 👌🏼

Comment thread crates/database/src/trades.rs
Comment thread crates/database/src/trades.rs Outdated
Comment thread database/README.md Outdated
@AryanGodara
AryanGodara self-requested a review September 23, 2026 13:56

@AryanGodara AryanGodara left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall LGTM
One (non-blocking) nit: the new jit test doesn't actually guard the branch-3 DISTINCT (I removed it and the test stayed green, the page-level DISTINCT masks it at limit 1000). Left a small-limit test on the thread that fails without the fix.

Comment thread crates/database/src/trades.rs
@AryanGodara
AryanGodara self-requested a review September 23, 2026 14:36
@MartinquaXD
MartinquaXD added this pull request to the merge queue Sep 23, 2026
@MartinquaXD
MartinquaXD removed this pull request from the merge queue due to a manual request Sep 23, 2026
@MartinquaXD
MartinquaXD added this pull request to the merge queue Sep 23, 2026
Merged via the queue into main with commit 67cd3a1 Sep 23, 2026
23 of 24 checks passed
@MartinquaXD
MartinquaXD deleted the reduce-load-from-trades-queries branch September 23, 2026 15:07
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 23, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants