migrations
migrations copied to clipboard
doctrine
Add `onMigrationsQueryExecuting` and `onMigrationsQueryExecuted` events
| Q | A |
|---|---|
| Type | improvement |
| BC Break | no |
| Fixed issues | - |
Summary
This PR adds new events onMigrationsQueryExecuting and onMigrationsQueryExecuted.
Use case
When you are using Mysql cluster and you alter big table, this operation blocks the whole cluster until the alter is done (blocking DDL operations). With event onMigrationsQueryExecuting you can check if the migration contains query with alter for big table and stop it before it executes with exception to avoid DB problems.
Big alters are then processed manually with https://www.percona.com/doc/percona-toolkit/LATEST/pt-online-schema-change.html
For more info: https://www.percona.com/blog/2014/11/18/avoiding-mysql-alter-table-downtime/
I'm not sure about this… how is this better than manually removing the line that should be executed? Won't this lead to situations where people believe there data has been migrated, when it really hasn't?
On one side, I really like this PR (especially the Query value object, and I thunk it should be used instead of the current arrays or arrays in the Executor and Migrator classes!).
Thank you.
On the other side I'm worried by the feature itself. Can't this be spotted during code review? or do you plan to use this as somme kind of "testing of migrations"?
I'm not sure about this… how is this better than manually removing the line that should be executed? Won't this lead to situations where people believe there data has been migrated, when it really hasn't?
@goetas @greg0ire The developer who is doing code review may not detect if the migration is problematic or not. He may not have permission to see the production database so he can't check table sizes. Also the size of the tables can change after the code review if the feature is not released ASAP but after some time (holidays).
This is the last line of defense that runs before the migration during the deployment process. If there is problem: the table is too big so it will block the whole cluster, the migration check stops it and the deployment fail. In this situation there is no confusion if something was done or not. If the migration query ran manually with specific tool, the executed migration version should be inserted manually in the migrations table too, so the next deployment tryout will skip it as already executed.
The migrations (for altering big tables in production) should NOT be removed, because in local or dev environments (with smaller databases) and tests it can run without any problems and no manual work is needed.
Anyway I am talking about the use case we will use the new event for. Other developers may find another good reasons for it, too.
I overlooked the fact that you would get an exception on attempting a migration on a big table, not just a silent removal of that particular line in the migration, so this sounds good 👍
@pulzarraider if you agree with https://github.com/doctrine/migrations/pull/906#discussion_r365561239, for me is good to go as new feature.
@greg0ire I think that if https://github.com/doctrine/migrations/pull/906#discussion_r365561239 is applied, migrations should behave as now, and users will have just one extra extension point in which the list of queries to execute is available, but can not be altered. Do you agree?
The example use case is specific to databases where alter statements can take a while and where projects have to provide a migration strategy if more than one big table has to change. Doctrine Migrations usually is a tool as a part of such a strategy, but not an out of the box strategy itself. So I'm not against the idea of introducing the new events.
@SenseException i'm against introducing events that alter the queries itself, not against events being able to monitor which queries are going to be executed, but looking better at the implementation of this pr, seems stat is not possible to change the executed queries. said that, then i'm also ok with this pr.
@pulzarraider can you rebase your pr?
Big alters are then processed manually
we're experimenting with running ALTER queries via pt-online-schema-change on the fly, but found zero extension points for it (other than postUp)
im not sure it's a good idea, but my first experiments worked well
to block such queries the SqlGenerator can be modified as well, hence im not sure these events are that useful :thinking:
IMHO what brings more value is to depend on a connection interface, rather than a concrete, in DbalExecutor
friendly ping :) our copy/paste of DbalExectutor just broke due updated upstream (TransactionHelper::rollbackIfInTransaction was added) :sweat_smile:
@ro0NL OP is not responding. If you want this change, why don't you open a new PR without the merge conflicts?
i dont want this change for an extension point :) i think DbalExecutor should depend on a connection interface