-
Notifications
You must be signed in to change notification settings - Fork 502
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
services/horizon/db2/history: Improve /trades DB queries performance #2869
Conversation
return q | ||
} | ||
|
||
q.rawSQL = fmt.Sprintf("(%s) UNION (%s) ", firstSQL, secondSQL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
take a look at TransactionByHash()
to see an alternative way to construct the UNION
query using squirrel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was my first approach but it doesn't work when you want to add ORDER BY
and/or LIMIT
to UNION
(requires brackets).
q.sql = q.appendOrdering(q.sql, op, idx, page.Order) | ||
} | ||
|
||
q.sql = q.sql.Limit(page.Limit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be moved to the else
clause?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, fixed in 013fdcb.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good. I just had 2 comments
This is cool! What's the overhead for migration to add the index look like for full pubnet history? |
@ire-and-curses I included this in the PR summary:
The new index is around ~1GB as far as I can remember. Can confirm tomorrow. |
Super. |
PR Checklist
PR Structure
otherwise).
services/friendbot
, orall
ordoc
if the changes are broad or impact manypackages.
Thoroughness
.md
files, etc... affected by this change). Take a look in the
docs
folder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
This commit improves performance of queries generated by
TradesQ
which is used in/trades
endpoint.First, a quick fix. A new multicolumn index was added on
base_asset_id, counter_asset_id, history_operation_id, order
. It supportsTradesForAssetPair
queries and improved the speed of such queries from ~5s on average in SDF cluster to a few ms. Adding the index took around 2 minutes in SDF cluster (with full history).Second, I updated queries that contain
base_* = X OR counter_* = X
conditions toUNION
s. Specifically,ForAccount
andForOffer
. Previous query was using a wrong index (htrd_pid
which required a lot of filtering) and adding new indices (base_
+history_operation_id
+order
) didn't really help (I believe because ofOR
statement). New query decreased execution times down to ms.Known limitations
github.com/Masterminds/squirrel
does not supportUNION
queries so it's built manually.scc
scenarios. A larger refactor is required to support mocking.