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: Improve performance of claimable balances queries #4690

Merged

Conversation

bartekn
Copy link
Contributor

@bartekn bartekn commented Nov 10, 2022

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 a new table claimable_balance_claimants which holds all claimants' destinations for corresponding claimable balances IDs. Also, improve other filters (by sponsor and asset) but adding better indexes for such queries.

Why

We noticed that "claimable balances for claimants" query (/claimable_balances?claimant=...) is very slow in Postgres 12. The reason, apart from possible changes to gin index on claimants field, is that the claimable_balances table size significantly increased in the last couple months.

Known limitations

This change requires a state rebuild to fill the new table. The new query has not been tested yet in stg because this environment is currently being rebuilt. I will test the query performance before merging the PR.

@bartekn bartekn marked this pull request as ready for review November 14, 2022 09:01
@bartekn bartekn requested a review from a team November 14, 2022 09:01
@bartekn
Copy link
Contributor Author

bartekn commented Nov 16, 2022

I was finally able to test the code in this PR in stg and the results aren't great.

In one of Horizon meetings I mentioned that we should avoid a mistake we made in history_transaction_participants and history_operation_participants and include ordering fields in the subtable used for outer queries (#1808). The code in this PR does not contain ordering fields because I was hoping that with limited number of balances per destination we could still make it fast with easier to maintain queries.

Unfortunately, while replaying production /claimable_balances?claimaint=... requests to staging I realized that there are destinations that are claimants on hundreds of thousands claimable balances. For example GCJOXRRZWS7KQU6QWG6KHSJR3RVRYPAQ3YZNO52EXNJCMJ67ERUISO2O is a destination on 610382 balances! This means that query engine must first get all the 610k rows, and the find them in the claimable_balances query and order by last_modified_ledger.

Another observation was that, contrary to history tables like participants mentioned above, current state tables are not immutable. This means that if a claimable balance entry changes (ex. by adding/changing a sponsor) all of it's destinations in a new table claimable_balance_claimants with ordering field will have to be updated as well. While this means up to 10 rows will be updated per operation, this can increase to 10,000 (technically slightly less because of sponsor sandwich ops) if all ops in a ledger change the sponsor.

This makes me think we have two options:

  • Add ordering field to claimable_balance_claimants risking potential performance security issues. (Please note that in the end the update can be fast, it's hard to tell without testing the actual ingestion).
  • Remove order by cb.last_modified_ledger [asc|desc] from the "Claimable Balances by Claimant" query. Technially this changes the contract because the order of results is different.

The second option is easier and, to be honest, I don't understand why we sort results by last_modified_ledger here but also in other state endpoints. Alphabetical order makes more sense because while it's still impossible to ensure we don't skip some entries (like balances created with IDs before the current page) we can at least walk through (almost entire) list.

Both solutions have pros and cons so decided to open a discussion here.

@bartekn
Copy link
Contributor Author

bartekn commented Nov 17, 2022

I decided to add order by field to claimable_balance_claimants in 73327c4 because it won't change the contract and also we still have plenty of time to test performance using verify-range. We can even write a small stress test. Once test pass, I'll deploy to stg.

I also noticed that "Claimable Balances for Asset" and "Claimable Balances for Sponsor" also have performance issues if an asset or sponsor is popular. I added new indexes to support such queries and fixed the query builder.

@bartekn
Copy link
Contributor Author

bartekn commented Nov 17, 2022

OK, initial tests in stg look very good. PTAL! I'll share detailed comparison report tomorrow.

@bartekn bartekn requested a review from a team November 17, 2022 22:33
@bartekn bartekn changed the title services/horizon: Improve performance of getting claimable balances by claimant services/horizon: Improve performance of claimable balances queries Nov 17, 2022
@bartekn
Copy link
Contributor Author

bartekn commented Nov 18, 2022

I started mirroring yesterday. Results look good, /claimable_balances endpoint is much faster (in chart: 90, 95 and 99 percentiles) and the number of 503s dropped to 0.

Screenshot 2022-11-18 at 13 10 11

@bartekn bartekn merged commit 5c83343 into stellar:master Nov 18, 2022
@bartekn bartekn deleted the improve-get-claimable-balances-claimants branch November 18, 2022 14:06
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.

2 participants