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 /accounts/{id}/transactions|payments|operations performance #2946

Closed
wants to merge 2 commits into from

Conversation

bartekn
Copy link
Contributor

@bartekn bartekn commented Aug 24, 2020

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

This commit improves the performance of /accounts/{id}/transactions|payments|operations endpoints by denormalizing history_operation_participants and history_transaction_participants. The feature is behind a feature flag because to work correctly a new set of indexes and full history reingestion are required.

Why

As explained in #1808 the set of queries made by "Transactions/Operations/Payments For Account" endpoints require denormalization of history_operation_participants and history_transaction_participants tables to include information about a transaction status (successful) and operation type (payment). This makes queries related to accounts with unusual operation distributions (ex. an account with a lot of offer operations but a few payment operations) fast.

Known limitations

  • This feature is experimental and behind a feature flag because it requires a full DB reingestion.
  • Indexes using new columns must be created manually (migration would be super slow on full history DBs and they wouldn't be able to use new feature without reingestion).
  • It adds new columns on participant tables that are set when new participants are inserted but ignored when reading data fro a DB if the feature flag is not set.

@bartekn bartekn requested a review from a team August 24, 2020 15:41
@cla-bot cla-bot bot added the cla: yes label Aug 24, 2020
@ire-and-curses
Copy link
Member

What is the storage overhead of the new columns and indices?

@bartekn
Copy link
Contributor Author

bartekn commented Aug 24, 2020

I will confirm it this week. I think new fields won't require much storage because these are just bools. For indexes I tried updating existing rows with in a test DB but it took ages and did not complete. Will confirm in a newly reingested DB.

@@ -213,11 +227,21 @@ func (p *ParticipantsProcessor) addOperationsParticipants(
return errors.Wrap(err, "could not determine operation participants")
}

for operationID, p := range participants {
for operationIndex, p := range participants {
operationID := toid.New(
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we constructing a toid here? it seems like the old code was not creating any toid

@bartekn
Copy link
Contributor Author

bartekn commented Nov 13, 2020

Closing for now. We can reopen it in the future when fixing all performance issues in a batch.

@bartekn bartekn closed this Nov 13, 2020
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.

Horizon: /account/:id/payments timeout error for accounts with large number of trades
3 participants