byjg / php-migration

Simple library writen in PHP without framework dependancy for database version control. Supports Sqlite, MySql, Sql Server and Postgres
MIT License
159 stars 27 forks source link

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

Open MrPetovan opened 4 years ago

MrPetovan commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Do you have any suggestion on how handle this?

MrPetovan commented 4 years ago

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 commented 4 years ago

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

Furgas commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Does this atomic metadata ignore only the partial status?

byjg commented 3 years ago

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.