Add onMigrationsQueryExecuting and onMigrationsQueryExecuted events

[pulzarraider]

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/

[goetas]

Thank you for your contribution.

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!).

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 did not review properly the code, will proceed after deciding if we want this feature.

@alcaeus @SenseException @greg0ire What do you think about introducing an event on each executed query?

[greg0ire]

How about using statement instead? queryString feels HTTP-esque

[greg0ire]

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?

[pulzarraider]

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.

[greg0ire]

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 👍

[greg0ire]

Please run vendor/bin/phpcbf to fix cs issues

[goetas]

I personally don't like that the check is done here. It is already too late, the migration process already started. Databases as MYSQL are not that good at transactions and altering the schema.

I would suggest to trigger a new event type right before the if (count($this->sql) !== 0) { check in the executeMigration method.
In that moment the execution of SQL commands did not start yet and is much safer to interrupt it.

[SenseException]

I would also expect that, when I consider the example use case, if a big table was the reason why an migration was aborted, that the previous executed queries of the same version on the smaller tables are rolled back. Maybe a test can cover that.

[goetas]

@pulzarraider if you agree with #906 (comment), for me is good to go as new feature.

@greg0ire I think that if #906 (comment) 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?

[greg0ire]

It does sound better!

[SenseException]

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.

[goetas]

@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?

[ro0NL]

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 🤔

IMHO what brings more value is to depend on a connection interface, rather than a concrete, in DbalExecutor

[ro0NL]

friendly ping :) our copy/paste of DbalExectutor just broke due updated upstream (TransactionHelper::rollbackIfInTransaction was added) 😅

[greg0ire]

@ro0NL OP is not responding. If you want this change, why don't you open a new PR without the merge conflicts?

[ro0NL]

i dont want this change for an extension point :) i think DbalExecutor should depend on a connection interface