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

Usage issue: Migration skips a version after an error #30

Open
MrPetovan opened this issue Sep 27, 2020 · 12 comments
Open

Usage issue: Migration skips a version after an error #30

MrPetovan opened this issue Sep 27, 2020 · 12 comments

Comments

@MrPetovan
Copy link
Contributor

I'm on version 5, I want to upgrade to version 8. The migration script for version 6 fails because the database was manually edited in-between. Trying to perform the same update yields Database was not fully updated. Use --force for migrate.. Using --force, version 7 and 8 scripts are applied but not version 6 again.

How can I apply version 6 again after an error?

I understand that this happens because the partial up status is recorded along with version 6 even if it wasn't applied at all, effectively staying in version 5, but this isn't recorded anywhere. I would expect version 5 to be recorded along with partial up and version 6 to be recorded when it has been successful, am I missing something in this library usage?

@byjg
Copy link
Owner

byjg commented Sep 28, 2020

It is always hard when a migration fails and there is no "correct" solution. As you described the error happened when you applied the version 6. The way I think to fix that is you apply a migration down from 6 to 5 e.g. migration update --up-to=5 --force. This will make your migration correct again. And then you can apply the migration up for the next versions (including the version 6).

Another thing: as you have another revisions to be applied (7 and 8) it means someone is having access write to the database and most probably because of it your script failed.

Anyway, that's a interesting discussion and I would love to hear more about it and suggestions.

@MrPetovan
Copy link
Contributor Author

Thank you for the answer, the problem is that if the 5-to-6 upgrade failed, it's possible the 6-to-5 downgrade will fail as well depending on the state of the database. I'm trying to provide a single command to automatically upgrade a database schema and I'm stumped on the double error scenario.

Maybe by giving a clear migration description to help troubleshooting the error?

What about having migration files being named XXXX_Upgrade_description.sql?

@byjg
Copy link
Owner

byjg commented Sep 28, 2020

Yes, you're correct. But when the migration fail is nothing much we can do in an automated way. We need to validate what happened, check if we won't lose data, etc. You can create a "dev" file to do the migration down - 0005-dev.sql, but anyway, is not that simple.

About the file name in this line https://github.com/byjg/migration/blob/master/src/Migration.php#L157 we can handle add a description in the filename. I particulary don't like this because it will open space to create 0005_Upgrade_decription.sql and 0005_another_upgrade_description.sql and it will require add some validations to deal with it.

One suggestion, is add the description as a metadata inside the file, something like that:

-- Description: Upgrade Description
-- Author: Joao
-- Data: 2020-09-28

...

What is in my plans is save the SQL checksum into the migration table. It will avoid issues if someone update an already applied migration sql. With this change I can "read" the metadata and make it available in case of errors, for example.

@MrPetovan
Copy link
Contributor Author

SQL file metadata would be fine with me, currently the exception message only indicates to run the command with --force but it doesn't say this particular migration will be skipped.

@byjg
Copy link
Owner

byjg commented Sep 28, 2020

Do you have any suggestion on how handle this?

@MrPetovan
Copy link
Contributor Author

I do. In the Migration->migrate() method, after the file name has been retrieved with Migration->getMigrationSql(), the file can be parsed for a description, and passed to Migration->callable_progress along the current version number.

@byjg
Copy link
Owner

byjg commented Sep 29, 2020

ok. I did the first implementation and created the PR #31

@Furgas
Copy link

Furgas commented Jun 8, 2021

I have just experienced the same problem - forcing migration when the state is partial up won't apply failed migration again. I always use transaction in my migration scripts so for me it would be sufficient to provide some flag in migrator (ex. $migrations_are_atomic) and change the line

$this->getDbCommand()->setVersion($currentVersion, Migration::VERSION_STATUS_PARTIAL . ' ' . ($increment>0 ? 'up' : 'down'));

to

if ($this->migrations_are_atomic === false) {
    $this->getDbCommand()->setVersion($currentVersion, Migration::VERSION_STATUS_PARTIAL . ' ' . ($increment>0 ? 'up' : 'down'));
}

For now I will just override migrate function and remove setting partial status.

@byjg
Copy link
Owner

byjg commented Jun 8, 2021

That's something interesting. I believe I can force always use transactions and ignore the partial status. But I am afraid about DDL changes in big tables. If the table has thousands of millions of data, it will require a lot of space for transaction and rollback, and not mentioned the fact the table will be unavailable for several minutes.

How do you guys would handle this scenario?

@Furgas
Copy link

Furgas commented Jun 8, 2021

You can't force to always use transactions because some databases ignore transactions when doing DDL (ex. Oracle). Maybe atomic flag in migration metadata could be introduced and then:

if ($fileInfo["atomic"] === false) {
   $this->getDbCommand()->setVersion($currentVersion, Migration::VERSION_STATUS_PARTIAL . ' ' . ($increment>0 ? 'up' : 'down'));
}

@byjg
Copy link
Owner

byjg commented Jun 8, 2021

Does this atomic metadata ignore only the partial status?

@byjg
Copy link
Owner

byjg commented Jun 8, 2021

Another point about the partial state: The main objective is to block the migrations applying over a database the migration failed. So, instead of ignoring the partial state, we can create a method to clear the state. Then we can fix the database manually and then clear the state, enabling us to apply the migration again.

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

No branches or pull requests

3 participants