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

Failed migrations can cause data loss. #98

Closed
wants to merge 1 commit into from

Conversation

larryb82
Copy link

This test shows that failed migrations can result in data loss. If a
migration fails, all earlier migrations are rolled back which can
destroy data. See the catch block in MigrationVersion->run() .

This test shows that failed migrations can result in data loss.   If a
migration fails, all earlier migrations are rolled back which can
destroy data.  See the catch block in MigrationVersion->run() .
@gmansilla
Copy link
Contributor

If you want to keep data then you should use the callbacks. Furthermore, you shouldn't be running migrations directly in a production database without testing first in a develop database.

@gmansilla gmansilla closed this Oct 30, 2012
@pacifists
Copy link

gmansilla I totally disagree with you. Why? Because it worked on our development boxes, worked on our staging box and on production it just dropped half of the database until it got fatal error itself and choked. Why that happened? Well because in callbacks model was used to manipulate data and it threw a php warning cause of single missing uploaded file ... so migrations backtracked and we had to restore our backups yesterday.

This behavior is not what I would expect from migrations plugin - to just destroy all data and try to run them again. The current behavior is dangerous and cost us some grief already. It just happened so that when I came looking for a solution this pull request was added.

@larryb82
Copy link
Author

@gmansilla, I hope you will reconsider this issue. I am working on a project that will be hosted by customers on their company intranet. At some point, my customers will need to install updates and I was hoping to use Migrations to manage changes to their database. Obviously, the code will be tested, but I cannot guarantee that the migration will run successfully. I don't want my customers to run into the same situation as @pacifists especially since they will be responsible for restoring the data.

@gmansilla, do you have any recommendations for using the callbacks to preserve the data?

@burzum
Copy link
Contributor

burzum commented Oct 30, 2012

@larryb82 I disagree, the purpose of a migration is not to create a backup. http://en.wikipedia.org/wiki/Data_migration#Database_migration For example if you have a huge db with several gb of data and you want to export it through a php script and write it somehow (serialized array?) to a file it becomes a huge and slow to process monster.

The far better solution would be to have a shell (bash) script that is triggered by the migration in the before and after callbacks using exec and is doing the backup by calling mysqldump (or other proper backup tools) and other commands if needed. You can return the file name by that script, catch that in the migration and if it fails use the backup file you just created to restore the database. That's the most clean solution IMO.

@gmansilla
Copy link
Contributor

@larryb82 What branch were you using? develop or master? What was the error in database that make migrations to rollback?

@pacifists
Copy link

@gmansilla , @burzum I was using master branch and I've got failing FK constraint due to our inheritance scheme, but the issue at hand is not data backup it's the data destroying if ANY php error occurs because there's a try arround 'run' and the catch that goes down all migrations. Why it can't be handled as other errors are like "would you like to abort or ignore this" instead? Right now it's really destructive and we have 6 months worth of migrations and now it just happened when deploying code to production.

Atm I don't feel secure using plugin for what we do because I have to do the impossible - be sure 100% that all the code written in callbacks under no circumstances throws errors otherwise I loose data again. And from experience I know that it's not possible to be 100% sure about such things, because we're just human.

@gmansilla
Copy link
Contributor

@larryb82 please do this and tell me your results:
Go and checkout any commit from Jul 17, 2012.
Run migrations and let the error with your FK to happen.
Post results here.

@larryb82
Copy link
Author

@gmansilla the commit from July 16, 2012 works as expected. An exception is thrown but it is not caught by run(), the failed migration is not added to the migrations table, and my data is not deleted. The logic in the catch block should be re-evaluated.

I've updated my test case to reflect this behavior.

@larryb82
Copy link
Author

@burzum interesting idea. I'll keep it in mind as I'm working on this project.

@burzum
Copy link
Contributor

burzum commented Oct 30, 2012

@larryb82 another point to always create a backup before doing something is this case: Everything works fine technically but somebody simply wrote a wrong migration that technically works and does not fail but causes wrong data, like truncating a field length by accident. Just truncate a 36 UUID by 1 char to 35 by accident, could be a typo for example and you have the mess. Doing something on a live db without doing a backup first is negligent.

Even if we would add some magical complete backup for all the supported datasources (we're not going to do that) this would not cover the case of human mistakes.

@gmansilla
Copy link
Contributor

@larryb82 The idea behind that feature was the following case (I don't know if it applies to your case)

  1. Migration containing changes to table A
  2. Migration containing changes to table B
  3. Migration containing changes to table C
  4. Migration containing changes to table D

Now, imagine there is a failure in migration number 3, let's say it was wrong FK (your case), there was no way to revert changes made in migration number 1 and 2.

#29

@larryb82
Copy link
Author

@gmansilla I want to fully understand this problem, so let me extend your example.

Assume, migrations 01 to 10 have been run successfully, the current version is 10.
11) Migration containing changes to table A
12) Migration containing changes to table B
13) Migration containing changes to table C
14) Migration containing changes to table D

Now, imagine there is a failure in migration number 13, let's say it was wrong FK (your case), there was no way to revert changes made in migration number 11 and 12.

As I understand the code, resetMigration() effectively runs 'down' on migrations 12, 11, 10,..., 01 but it only needs to run 'down' on migrations 12 and 11. Couldn't this be fixed by passing $latestVersion to resetMigration?

@gmansilla
Copy link
Contributor

#29

deizel pushed a commit to deizel/cakedc-migrations that referenced this pull request Mar 4, 2016
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.

4 participants