Expose support for transactional DDL or lack thereof

[greg0ire]

Q	A
Type	feature
BC Break	no
Fixed issues	n/a

Summary

Clients may need that information, notably in the context of SQL
migrations.

At the moment, I only know about this being an issue with MySQL, there might be more platforms having issues with this.

The MySQL docs I linked in the code say the following:

Similarly, DDL statements are not transactional, and therefore a transaction is (almost) never started for a DDL statement. But there's a difference between a DDL statement and an administrative statement: the DDL statement always commits the current transaction (if any) before proceeding; the administrative statement doesn't.

I did not add unit tests for this method since there is no complexity, do tell me if you feel they are needed anyway.

[greg0ire]

Blocked by #4482

[stof]

According to https://www.datasunrise.com/blog/professional-info/how-popular-rdbmss-deal-with-ddl-commands-in-transactions/ Oracle does not support it either.

[stof]

I did not add unit tests for this method since there is no complexity, do tell me if you feel they are needed anyway.

should we actually have a functional test validating that this method does not lie ? This could be done by starting a transaction, creating a table, rolling back the transaction and then checking if the table exists or no (if transactional DDL works, there should be no such table left)

[morozov]

The API in terms of "supports" as opposed to "do something" violates the "Tell, Don't Ask" principle. It moves the responsibility of handling the difference in implementation details to the consumer of the API and creates an unnecessary coupling.

A proper API, in this case, should look like e.g. beginTransactioWithDDL() which would fail for MySQL.

[greg0ire]

I don't think this is compatible with @stof's plan from doctrine/migrations#1104 (comment) :
The migrations package would still need a way to decide whether to call beginTransaction() (for the case of a migration purely about data), or beginTransactionWithDDL() (for the main case).

[morozov]

If the migration becomes transactional or non-transactional based on the underlying platform, it invalidates the purpose of the DBAL: the end-user will experience different results on different platforms. They test it on Postgres, it works as expected; they test it on MySQL, it applies partially in case of a failure. Is it the desired behavior?

[greg0ire]

One could argue on what might be considered different results. I think it might be sensible to say people using extends AbstractionMigration expect their migration to just work and don't particularly care whether is transactional or not, while people overriding isTransactional to let it return true really care about their migration being transactional. Your comment might make us reconsider if the behavior of the code that generates migrations should be changed and only this one, so that AbstractionMigration::isTransactional() still returns true in every case, but is overrided to return false when MySQL is detected. @stof, what do you think?

[greg0ire]

cc @goetas

[morozov]

On a more constructive note: the problem I've been observing about the ORM and now Migrations is that the consumers of the DBAL APIs are trying to force the introduction of certain APIs in the DBAL that make sense only for them. All the supports* and prefers* platform methods are a sign of that.

We need to find a different approach to extending the platform hierarchy to the needs of a given project w/o stuffing new responsibilities into the DBAL. This way, each project will be able to maintain that extended API according to its needs and the release cycle w/o creating unnecessary coupling.

The closest possible solution that comes to mind is the Visitor pattern. The DBAL may define the PlatformVisitor interface where each platform could identify itself, and the implementor of the visitor could define the actual platform-dependent logic. The only downside of that is the interface will have to remain intact between the major releases and won't be able to accommodate new platforms. Also it's unclear how fine-grained it should be (e.g. whether MySQL, MySQL 8 and MariaDB are different platforms from this PoV). In any event, it's just an example.

[stof]

well, if you suggest that other Doctrine projects need to implement some code which has a switch on the platform to perform the logic, I find this even worse: adding a new platform (in DBAL or as a separate package providing an implementation) becomes a BC break for these projects. The list of platform implementations becomes part of the API for such case.
I think this idea that DBAL APIs should only do things and not report capabilities of the platform is too limiting (or would require upstreaming lots of logic into DBAL itself instead of allowing downstream projects do their thing).

If Migrations need to implement a different behavior based on the platform, it should be solved in Migrations. The DBAL is about common things between the platforms, not about the different things.

That's precisely what's being done today: doing the different logic based on the capabilities of the platform.. But for that, we need to know those capabilities. And if the DBAL platform stops supporting that, these projects would need to implement a separate concept of platforms which either needs to be configured explicitly (harder for user as they have to configure multiple times they use Postgresql and they could end up with a mismatch if they are not aware of that) or needs the DBAL platform to return the corresponding implementation (no-go for the coupling direction) or requires these projects to support a fixed list of DBAL platforms (the ones for which it knows the correspondence).

[morozov]

well, if you suggest that other Doctrine projects need to implement some code which has a switch on the platform to perform the logic, I find this even worse

I believe this is the best pragmatic approach in this case:

if ($platform instanceof MySQLPlatform) {
    // disable transactions
}

The list of platform implementations becomes part of the API for such case.

I agree that it's not a way forward. It was just an example of how hierarchies can be extended from outside.

I think this idea that DBAL APIs should only do things and not report capabilities of the platform is too limiting (or would require upstreaming lots of logic into DBAL itself instead of allowing downstream projects do their thing).

I don't think so. The DBAL is not a source of truth of the platform's capabilities. It just does its best to fulfill its APIs using certain platform features. The meaning of "supports transactional DDL" may have nuances depending on the use case of which the DBAL itself has none. That's why I'm strongly opposed to adding such methods.

That's precisely what's being done today: doing the different logic based on the capabilities of the platform.

It's the design choice of Migrations to fulfill certain API calls in a different way based on the platform. It cannot be dictated by the DBAL which is an abstraction layer.

projects would need to implement a separate concept of platforms

This is absolutely fine. Transaction handling and data migrations are two different bounded contexts. The same model (a platform) may have different representations in each context to solve certain problems. The only question is how to extend the model that belongs to one context in another. The best approach I see for now is if ($platform instanceof MySQLPlatform) in Migrations.

[kimhemsoe]

This is absolutely fine. Transaction handling and data migrations are two different bounded contexts. The same model (a platform) may have different representations in each context to solve certain problems. The only question is how to extend the model that belongs to one context in another. The best approach I see for now is if ($platform instanceof MySQLPlatform) in Migrations.

@morozov I can see where you going and i would agree with you if it was not because Migrations, ORM and DBAL is all under the same umbrella called Doctrine. It should be possible to implement a new driver, platform and schema for a unsupported vendor in dbal without without the need to also hack into Migrations, ORM and etc. to make the driver work in those. That have always, in my humble opinion, been one of the goals behind DBAL and Doctrine as a whole.

[morozov]

I heard this idea expressed by others and I disagree with the approach. While we should work together on solving problems, each library should have its boundaries. The above is a justification of bad design decisions based on the organizational structure.

Here's another example of such an issue: #4335 and another one: #3980 (review). Both of them were solved the right way w/o crippling the DBAL API.

[greg0ire]

Reusing this PR to see if an AppVeyor issue spotted elsewhere also affects this branch.
UPD: 3.1.x is not affected somehow

[mvorisek]

It should be false by default and covered with functional test run againt every DB vendor in CI

[ostrolucky]

I've created #4544 which is the test case better demonstrating the issue. Interestingly, issue is in pdo_mysql, but not mysqli. We need DBAL to handle this seamlessly if it's undesired for DBAL to add API that consumer could use to solve this problem.

[greg0ire]

Closing as this has been resolved in doctrine/migrations