Migration transaction mode not applicable to MySQL/MariaDB

[michaelbromley]

Documentation Issue

What was unclear or otherwise insufficient?

Docs in question:

• Migrations guide https://github.com/typeorm/typeorm/blob/master/docs/migrations.md

  Note: this operation uses SQL's TRUNCATE query which cannot be reverted in transactions.

• typeorm/src/migration/MigrationExecutor.ts

  Lines 22 to 28 in 6ce65fb

  	/**
  	* Indicates how migrations should be run in transactions.
  	* all: all migrations are run in a single transaction
  	* none: all migrations are run without a transaction
  	* each: each migration is run in a separate transaction
  	*/
  	transaction: "all" | "none" | "each" = "all";

• typeorm/src/connection/Connection.ts

  Line 286 in 6ce65fb

  async runMigrations(options?: { transaction?: "all" | "none" | "each" }): Promise<Migration[]> {

Though not discussed in detail either in the docs or in the inline source comments, from the above documentation (as well as from reading a number of issues here, e.g. #4629, #2693, I was under the impression that any errors in a migration would cause the migration to be rolled back.

However, this is not supported by MySQL / MariaDB, as I eventually discovered after spending a long time trying to figure out why failed migrations left my MariaDB database in a broken state.

The root cause is that MySQL does not support rollback of DDL statements (ALTER TABLE etc).

References:

• Bug Report: ALTER TABLE does not roll back (https://bugs.mysql.com/bug.php?id=28727)
• Stack Overflow: Transactional DDL workflow for MySQL (https://dba.stackexchange.com/q/90794)

This fact is really important to know when choosing which DBMS to go with!

Recommended Fix

There is no fix for the problem of failed migrations in MySQL. Therefore I think the best we can do is make it clear in docs and in code / run-time behavior that transactions for migrations do not work in MySQL/MariaDB.

1. In the Migrations doc, add a section about how migrations are performed in a transaction
   • Add some docs about the transaction modes "all" | "none" | "each" and what they mean
   • Include a clear indication of which DB drivers support rollbacks, so users know what to expect.
2. Extract the type "all" | "none" | "each" (which appears 6 times in the code base) into a named type, with a comment describing what the modes mean and also a note about which drivers support these modes.
3. Add a run-time warning when generating / running migrations when a non-transaction-supporting driver is detected. Something like:

   Warning: [driver] does not support transactions for migrations. Any errors encountered when performing the migration may leave your schema in an inconsistent state.

Relevant Database Driver(s)

• aurora-data-api
• aurora-data-api-pg
• better-sqlite3
• cockroachdb
• cordova
• expo
• mongodb
• mysql
• nativescript
• oracle
• postgres
• react-native
• sap
• sqlite
• sqlite-abstract
• sqljs
• sqlserver

Are you willing to resolve this issue by submitting a Pull Request?

• Yes, I have the time, and I know how to start.
• Yes, I have the time, but I don't know how to start. I would need guidance.
• No, I don't have the time, although I believe I could do it if I had the time...
• No, I don't have the time and I wouldn't even know how to start.

[imnotjames]

I think it should be an error, not a warning. A warning can be ignored very easily. This is asking the driver to do something it can't, and a very dangerous case at that. Thoughts?

[michaelbromley]

Yes, that's a good point. When we are talking potential data loss it is preferable to fail with an error before the migration can even run.

So the question then is how to handle this when running a migration from the CLI. Right now, the "transaction" option defaults to "all":

typeorm/src/commands/MigrationRunCommand.ts

Line 58 in 4f7481b

transaction: "all" as "all" | "none" | "each",

So we could add a check in Connection.runMigrations() which inspects the current driver type and throws an error:

/**
 * Runs all pending migrations.
 * Can be used only after connection to the database is established.
 */
async runMigrations(options?: { transaction?: "all" | "none" | "each" }): Promise<Migration[]> {
    if (!this.isConnected)
        throw new CannotExecuteNotConnectedError(this.name);
    const migrationExecutor = new MigrationExecutor(this);
    migrationExecutor.transaction = (options && options.transaction) || "all";

+   switch(this.options.type) {
+     case 'mysql':
+     case 'mariadb': {
+       if (migrationExecutor.transaction !== 'none') {
+         throw new Error(`${this.options.type} does not support transactions for migrations. Set the "transaction" option to "none" and make sure to test your migration against non-production data first.`);
+       }
+   }
    const successMigrations = await migrationExecutor.executePendingMigrations();
    return successMigrations;
}