golang-migrate / migrate

Database migrations. CLI and Golang library.
Other
15k stars 1.38k forks source link

TRUNCATE and MySQL driver. #584

Open martinarrieta opened 3 years ago

martinarrieta commented 3 years ago

Describe the Bug MySQL does not support DDL statements inside a transaction, so, when you drop all the rows inside the migrations table with a TRUNCATE, MySQL will automatically commit that transaction and create a new one for the insert that is after that.

Also, there are some issues with truncate and MySQL 5.7 when you have a large buffer pool that is not solved yet. The bug report was reported for the version 5.5.28 but in the TRUNCATE documentation say that it is still affect the latest 5.7 version.

Steps to Reproduce Steps to reproduce the behavior:

  1. Setup golang-migrate inside a large MySQL instance (buffer pool bigger than 150G)
  2. Run the migrations multiple times
  3. See how MySQL stalls.

Expected Behavior I'd like to change that TRUNCATE for a DELETE, which is transactional and for a small number of rows it is not an issue.

Migrate Version v4.14.1

Loaded Database Drivers MySQL

Additional context Add any other context about the problem here.

dhui commented 3 years ago

Thanks for reporting the issue!

For reference, the MySQL 8 docs:

In MySQL 5.7 and earlier, on a system with a large buffer pool and innodb_adaptive_hash_index enabled, a TRUNCATE TABLE operation could cause a temporary drop in system performance due to an LRU scan that occurred when removing the table's adaptive hash index entries (Bug #68184). The remapping of TRUNCATE TABLE to DROP TABLE and CREATE TABLE in MySQL 8.0 avoids the problematic LRU scan.

m00dawg commented 3 years ago

Other thing to point out though, even with that fix, TRUNCATE also messes around with the filesystem more than a DELETE. We ran into this with the migrations table itself, which only contained a single row. That means, even if TRUNCATE "works" it will be slower than a DELETE for a single row and doesn't provide any other benefits (like reclaiming disk space) as a result. I think it's worth of evaluating when/where DELETE vs TRUNCATE should be used in the tool (and I'd definitely recommend DELETE in the case of the migration table, or any other tables with a mere handful of rows).

dhui commented 3 years ago

What about replacing TRUNCATE with DROP TABLE + CREATE TABLE, which is what MySQL 8 does according to the docs?

m00dawg commented 3 years ago

That'll avoid the issue pre-8.0 (e.g. 5.7) though still does file operations you can avoid with the DELETE. I should be clear, DELETE is much worse performance-wise when the table is large, so I'm mostly referring to the performance differences for small tables (like the migration table)

martinarrieta commented 3 years ago

Also, CREATE and DROP table are not transactional in MySQL (all versions), so that means that if you use any DDL, MySQL will automatically commit the transaction and it will leave the insert that is after that statement in a different transaction.

dhui commented 3 years ago

Given that the migration table is small, we could run DELETE + INSERT in the same transaction. The advisory lock should prevent any race conditions. I don't think the transaction provides much value unless the isolation level is SERIALIZABLE.

dhui commented 3 years ago

@martinarrieta also mentioned:

Also, delete will create a lock over all the rows in the table, but not a TABLE LOCK, if that is what you need (lock the table), you could use LOCK TABLES instead but I don't think that it is really needed because all other transactions will be waiting for the rowlock anyway.

Let's try running DELETE + INSERT in a single SERIALIZABLE transaction.

cinaglia commented 3 years ago

I ran into this issue today. Our database migration tool relied on there always being records in the schema_migrations table, which was not the case for a very brief period (between the TRUNCATE and the INSERT statements).

The solution proposed in https://github.com/golang-migrate/migrate/pull/585 seems like a reasonable one to me. Is there anything blocking it from being merged? Let us know if there anything we can do to help move things along.

dhui commented 3 years ago

@cinaglia the transaction needs to be made SERIALIZABLE

dhui commented 2 years ago

Fixed by https://github.com/golang-migrate/migrate/pull/656. Will close this issue after the next release. In the meanwhile, if you're impacted by this issue, use the master branch.