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

Make the migrations not suck #1793

Closed
vito opened this issue Nov 7, 2017 · 7 comments
Closed

Make the migrations not suck #1793

vito opened this issue Nov 7, 2017 · 7 comments

Comments

@vito
Copy link
Member

vito commented Nov 7, 2017

Here are some annoyances with our current migration stack:

  • They all have numbered filenames, where the number doesn't matter, and you have to remember to add it to the slice in the right place anyway. We've forgotten to add it to the slice in the past, and there's no way to notice (sometimes it's just an optimization).
  • We're now hitting build failures on Windows because (we think) we have so many darn files or exported functions or something in that package. See bin-rc #893.
  • There's no support for "down" migrations, making it impossible to downgrade Concourse.

Here are some things we have to get right:

  • Migrations should never run concurrently. We have our own code to ensure this via database locking.
  • Migrations should run transactionally. This is kind of obvious. We should decide what the bounds of the transaction should be though (i.e. per-migration-step or per-entire-migration).

Here are some goals for whatever we do to fix this:

  • Down migrations. If possible. Being able to downgrade Concourse is always nice. How hard would this be for some of our existing migrations?
  • Using an external library/package would be great.
  • Not having to maintain a list of migrations in addition to the filenames.
  • Some obvious way to make a new migration.
  • It would be nice to be able to squash the migrations every now and then, so on initial start the whole schema is bootstrapped at once.
  • It would also be nice to be able to see the schema all at once, as a developer. This could be related to the above point, if after running the migrations it generates a schema file that is then used to bootstrap, or something. (This could make merge conflicts more common though...)
@xtremerui
Copy link
Contributor

Have we considered using goose or mattes/migrate

They both support down migrations and have lots of features we might need

@christophermancini
Copy link

https://github.com/mattes/migrate appears to be younger yet nearly have twice the contributors and more consistent activity. It also supports more than sql which might be useful down the road. This would be my vote between the two.

@vito
Copy link
Member Author

vito commented Nov 8, 2017

Another thing to note: a lot of these tools just have you write .sql files, which is great and all since it's simple, but we've had some pretty hairy migrations in the past that require lots of Go code.

We should consider what we would have to do if all we could write was .sql. Maybe a simple migration that does the schema work, and then a batch operation that happens on startup, and we keep track of these batch operations separately?

@jwntrs
Copy link
Contributor

jwntrs commented Nov 8, 2017

Im not sure we can get away with running all the .sql migrations up front and then batch operations at startup. It seems like we would run into problems if we needed to do something like create a table, migrate data (with complicated logic that can't be done in sql), then drop a table. Or would the dropping be part of the batch operation? This then seems weird because we have schema changes in multiple places.

@christophermancini
Copy link

I think it is worth noting that the mattes/migrate project is considering the ability to have a shell migration driver. mattes/migrate#171

@jwntrs
Copy link
Contributor

jwntrs commented Nov 8, 2017

Yeah it looks like https://github.com/mattes/migrate is the winner. We're going to go with .sql only migrations for now and look at introducing batch operations if/when we need them.

We are also planning to collapse all pre-3.6 migrations into a bootstrap schema. This means all migrations from pre-3.6 deployments will have to migrate to 3.6 first.

jwntrs pushed a commit that referenced this issue Nov 17, 2017
#1793

Submodule src/github.com/concourse/atc 4041978..e5f8a19:
  > Acquire a lock when running database migrations
  > Fix transaction in migration test
  > Add migration for updating resource_caches unique constraint
  > Remove resource_spaces migration since it got reverted
  > Fix down migration issues
  > Fix issue with resource_spaces_id_seq
  > Run migrations in transactions
  > Do not truncate new version table in test helper
  > Add test helpers for migration refactor
  > use mattes/migrate for database migrations
  > create bootstrap schema from v3.6.0
Submodule src/github.com/mattes/migrate 000000000...69472d5f5 (new submodule)

Signed-off-by: Rui Yang <ryang@pivotal.io>
@vito vito removed the chore label Nov 27, 2017
@vito
Copy link
Member Author

vito commented Nov 27, 2017

I didn't see anything for the "Some obvious way to make a new migration" goal, so I went ahead and added a script:

./scripts/create-migration some_migration_name

All this does is install the migrate cli and run create with the appropriate flags.

I haven't decided yet where to document this (CONTRIBUTING.md starts to feel unwieldy; maybe a wiki is a better approach?), but at least the script's there.

Accepting!

@vito vito closed this as completed Nov 27, 2017
@vito vito added the accepted label Nov 29, 2017
@vito vito modified the milestone: v3.7.0 Nov 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants