Skip to content
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/internal/db2: Add column and index to improve trade_type filter performance #4149

Merged
merged 6 commits into from
Dec 21, 2021

Conversation

tamirms
Copy link
Contributor

@tamirms tamirms commented Dec 19, 2021

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    services/friendbot, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated any docs (developer docs, .md
    files, etc... affected by this change). Take a look in the docs folder for a given service,
    like this one.

Release planning

  • I've updated the relevant CHANGELOG (here for Horizon) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    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.

What

Add trade_type column which is an enum value that represents whether the trade occurs on the orderbook or against a liquidity pool. The trade_type column is populated during ingestion. This commit also adds an index on the trade_type column to make queries for liquidity pool trades fast.

Why

Requests to the /trades?trade_type=liquidity_pool endpoint timeout because the query to fetch liquidity pool trades is too slow.

Known limitations

It took 33 minutes on staging to apply the migration. Also, the size of the htrd_by_trade_type index is 3278 mb.

I considered removing the trade_type filter functionality from the trades endpoint but, within the past week, we have received horizon requests to the trades endpoint with trade_type set to orderbook. So, it seems that we would be breaking clients by removing this functionality.

@tamirms tamirms requested a review from a team December 20, 2021 08:47
ALTER TABLE history_trades ADD trade_type smallint DEFAULT 1 CHECK(trade_type > 0);
UPDATE history_trades SET trade_type = 2 WHERE base_liquidity_pool_id IS NOT NULL OR counter_liquidity_pool_id IS NOT NULL;
CREATE INDEX htrd_by_trade_type ON history_trades USING BTREE(trade_type, history_operation_id, "order");

Copy link
Contributor

Choose a reason for hiding this comment

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

would it be ok to drop the other liquidity pool id indexes on history_trades? not sure if they come into play on other queries, but they aren't used for deriving trade type query anymore, correct? htrd_by_base_liquidity_pool_id htrd_by_counter_liquidity_pool_id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is an endpoint to fetch all trades occurring on a specific liquidity pool (e.g. https://horizon.stellar.org/liquidity_pools/001403bfa898ef65015bb585e07343811aca01e02cc2a6f126a6c842f6d5359c/trades ) . those indexes are used by that endpoint

Copy link
Contributor

@sreuland sreuland left a comment

Choose a reason for hiding this comment

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

nice improvement.

@bartekn
Copy link
Contributor

bartekn commented Dec 20, 2021

I wonder if we can reuse existing fields in the index. 30m migration sounds like a long one (but I think we had similar ones in the past).

@tamirms
Copy link
Contributor Author

tamirms commented Dec 21, 2021

@bartekn I tried adding different indexes without adding a new trade type column, for example:

CREATE INDEX htrd_base_liquidity_pool_id_with_order ON history_trades USING BTREE(base_liquidity_pool_id, history_operation_id, "order");
CREATE INDEX htrd_counter_liquidity_pool_id_with_order ON history_trades USING BTREE(counter_liquidity_pool_id, history_operation_id, "order");

But unfortunately those indexes were not effective in speeding up the query.

@tamirms tamirms merged commit a58b746 into master Dec 21, 2021
@tamirms tamirms deleted the trade-type branch December 21, 2021 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants