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: Lock ingestion while applying DB migrations #4587

Merged
merged 10 commits into from
Sep 27, 2022

Conversation

bartekn
Copy link
Contributor

@bartekn bartekn commented Sep 14, 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

Acquire ingestion lock before applying DB migrations to stop ingestion.

Why

When applying 58_add_index_by_id_optimization.sql migration file we experienced a deadlock. It can happen when ingestion is running because ingestion by inserting or updating certain rows can acquire RowExclusiveLock which conflicts with other locks (like ShareLock when creating a new index).

Close #4588.

Known limitations

We lock ingestion only for up migrations. down migrations can remove key-value table which can lead to deadlocks.

Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

I think that the implementation is almost there - I've added few comments.

return 0, err
}

tx, err := txConn.BeginTx(context.Background(), nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

given that you've already used BeginTx instead of the "classic" Begin, why don't you take advantage of that and use the TxOptions ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Conn only exposes BeginTx method.

services/horizon/internal/db2/schema/main.go Outdated Show resolved Hide resolved
services/horizon/internal/db2/schema/main.go Outdated Show resolved Hide resolved
@bartekn bartekn marked this pull request as ready for review September 27, 2022 13:33
@bartekn bartekn requested a review from a team September 27, 2022 13:33
@bartekn
Copy link
Contributor Author

bartekn commented Sep 27, 2022

@stellar/horizon-committers PTAL, tests pass and I applied changes suggested by @tsachiherman.

@bartekn bartekn merged commit 1584aa3 into stellar:master Sep 27, 2022
@bartekn bartekn deleted the lock-ingestion-db-migration branch September 27, 2022 14:38
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.

Debug deadlock while applying DB migrations
2 participants