Migrations are piling up bogus savepoints on MySQL/MariaDB with PDO_MySQL

[derrabus]

Bug Report

Q	A
BC Break	no
Version	3.4.7

Summary

I've run into this issue in multiple projects that connect to a MySQL or MariaDB through the PDO_MySQL driver with nested transactions (aka savepoints) enabled. MySQL has this bad habit of silently committing a transaction when receiving a DDL statement. This is a problem because we wrap all migrations into a transaction by default.

Since PDO throws when we attempt to commit a transaction although none is active, we politely ask PDO if a transaction is active and if that's not the case, we don't commit.

migrations/lib/Doctrine/Migrations/Tools/TransactionHelper.php

Lines 59 to 66 in 76a1573

	private static function inTransaction(Connection $connection): bool
	{
	$innermostConnection = self::getInnerConnection($connection);
	/* Attempt to commit or rollback while no transaction is running
	results in an exception since PHP 8 + pdo_mysql combination */
	return ! $innermostConnection instanceof PDO || $innermostConnection->inTransaction();
	}

That however is a problem because DBAL tracks the transaction nesting level and is unable to detect the silent commit. Thus, after the first migration was executed, the DBAL connection still believes we're in a transaction.

Now, when the second migration is started, we open a transaction again. Since DBAL believes we're already in a transaction, it will attempt to create a savepoint instead of starting a transaction. Creating a savepoint outside of a transaction of course doesn't make much sense, so MySQL (you guessed it) silently ignores that savepoint. So, DBAL believes we're at nesting level 2 while we're actually still at zero. Fun times.

At the end of the second migration, we ask PDO again if a transaction is active. That's not the case, so we don't commit.

As you can tell, this goes on an on for each migration, and after 200 migrations, the DBAL connection believes we're inside an active transaction with 199 savepoints.

The consequence is that if you attempt to use an actual transaction inside a migration or reuse that connection after the migrations have run and attempt to open and commit a transaction, DBAL will raise an exception similar to this:

SQLSTATE[42000]: Syntax error or access violation: 1305 SAVEPOINT DOCTRINE_43 does not exist

This issues goes away if in each migration (that issues DDL statements) I override the isTransactional() method like this:

public function isTransactional(): bool
{
    return false;
}

On a MySQL database, this is 100% correct because executing a migration with DDL statements inside a transaction is futile. However, this fix is absolutely not obvious and people will keep forgetting to do so in future migrations.

The issue also goes away if I switch to the MySQLi driver: Unlike PDO, MySQLi does not complain about a transaction not being active, so we ask the DBAL connection to commit although that's a no-op after a DDL statement. It does however make sure, the DBAL connection's transaction nesting level is in sync.

I don't know how a good solution to this problem looks like, we need to revisit #1131 once again.

cc @ostrolucky @greg0ire

[derrabus]

One idea: We deprecate not overriding isTransactional() and make it abstract in 4.0. When generating a migration, we check the platform so we can generate a good default implementation (returning false for MySQL/MariaDB, true for all other platforms).

[greg0ire]

The second part of your proposal seems enough. We could even generate the override only when in the false case (after all false is the exception, not the rule, right?), with a link to the documentation explanation about this.

[derrabus]

The second part of your proposal seems enough.

I'm not sure. Returning true here does not seem like a good default, given the trouble that this default has caused in the past.

[greg0ire]

Do you mean it has caused trouble for RDBMSs other than MySQL?

[derrabus]

Do you mean it has caused trouble for RDBMSs other than MySQL?

No. 😅

[greg0ire]

Ok, then if we generate a method that returns false for PDO+MySQL or whatever the right condition is, I'd say we're assuming users of any other systems expect their migrations to be transactional. I think it's a fair assumption to make and that there is no need to pollute their migration files with a method reminding them about it every time.

[derrabus]

Seems fair.

[BusterNeece]

One question about this since it appears I had the same issue (in #1413): is there no longer a config setting in Migrations itself for transactional? I looked through the code for executing migrations and it was only looking in the migration classes themselves for that boolean value, not in the config passed to migrations during setup.

That seems counter to the docs that suggest that you can provide transactional => false in the global config to disable it for all migrations. In my investigations, that doesn't seem to be the case.

The solution for me was to build out a custom template that migrations were based on that inherited from an abstract migration class, which then had the isTransactional function return false. It's a lot of work that could've totally been avoided if that config value worked.

[greg0ire]

That's because it is used at migration generation time, not migration execution time. See 3fbbca6 for more details…

Hang on… 🤔 … @derrabus @BusterNeece did you set transactional to false in your config? I had completely forgotten I had contributed that.

It's a lot of work that could've totally been avoided if that config value worked.

Pretty sure it works… it's just that it does not work the way you think it works 😉 If you can think of documentation improvements to clarify that, please do send a PR.

[BusterNeece]

@greg0ire I think it definitely could stand to be clarified in the documentation that the transactional flag refers to generating migrations, not applying them.

I already have a ton of existing migrations and for some reason, upgrading to ORM 3 and DBAL 4 seems to have caused this kerfuffle as far as savepoints backing up, because it wasn't happening before, so I had to go back and set all of my existing migrations to extend from that abstract class that had it defined (as they weren't extending any class previously). So unfortunately I don't think even that config setting could've saved me any trouble here.

[greg0ire]

I think it definitely could stand to be clarified in the documentation that the transactional flag refers to generating migrations, not applying them.

Well the last paragraph of this page of documentation definitely mentions that in a pretty clear way. If you feel something else needs to be improve, please send a pull request.

Regarding the upgrade to ORM 3 and DBAL 4, it sounds kind of weird to me that it is the cause for your issue, or at least, it sounds weird to me that you did not have another issue before, since with PHP 8, ORM 2 and DBAL 3, I would expect you to stumble upon this deprecation and address it:

migrations/lib/Doctrine/Migrations/Tools/TransactionHelper.php

Lines 24 to 28 in 76a1573

	Context: trying to commit a transaction
	Problem: the transaction is already committed, relying on silencing is deprecated.
	Solution: override `AbstractMigration::isTransactional()` so that it returns false.
	Automate that by setting `transactional` to false in the configuration.
	More details at https://www.doctrine-project.org/projects/doctrine-migrations/en/stable/explanation/implicit-commits.html