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: require COUNT param for horizon db migrate commands #3737

Merged

Conversation

paulbellamy
Copy link
Contributor

@paulbellamy paulbellamy commented Jul 1, 2021

Fixes #3720

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

Require the COUNT param for horizon db migrate down COUNT (and redo, to prevent accidentally running all downward migrations. up is unchanged, so you can run all upwards migrations.

Add a horizon db migrate status command, printing migrations and the time they were applied. A short-term fix to make figuring out the count possible. Output like:

$ horizon db migrate status
Migration                                                       Applied
1_initial_schema.sql                                            2021-06-29 16:29:19.728113 +0000 UTC
2_index_participants_by_toid.sql                                2021-06-29 16:29:19.729313 +0000 UTC
3_use_sequence_in_history_accounts.sql                          2021-06-29 16:29:19.730471 +0000 UTC
4_add_protocol_version.sql                                      2021-06-29 16:29:19.731004 +0000 UTC
5_create_trades_table.sql                                       2021-06-29 16:29:19.732911 +0000 UTC
6_create_assets_table.sql                                       2021-06-29 16:29:19.734534 +0000 UTC
7_modify_trades_table.sql                                       2021-06-29 16:29:19.737889 +0000 UTC
8_add_aggregators.sql                                           2021-06-29 16:29:19.739113 +0000 UTC
...

Why

It's a bit of a footgun at the moment. The default behaviour of horizon db migrate down is maximum destruction.

Known limitations

  • Figure out where to document this. (especially using status to figure out how many steps to migrate down). Suggestions welcome...

@paulbellamy paulbellamy requested a review from a team July 1, 2021 10:57
@2opremio
Copy link
Contributor

2opremio commented Jul 5, 2021

I think that for the migrate status command it would be really useful for each row to also print what migrate down/up COUNT command is needed to get there. I can create a separate ticket for that if you prefer.

Copy link
Contributor

@2opremio 2opremio left a comment

Choose a reason for hiding this comment

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

Optionally, it would be useful to have a integration test for the migration commands.

@paulbellamy
Copy link
Contributor Author

Opened #3744 to track any improvements to horizon db migrate status

@paulbellamy paulbellamy merged commit fb81143 into stellar:master Jul 6, 2021
@paulbellamy paulbellamy deleted the paulb/3720-require-migrate-count branch July 6, 2021 12:27
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.

services/horizon: Make migrate up/down [COUNT] param mandatory to prevent wiping your db
2 participants