dbal icon indicating copy to clipboard operation
dbal copied to clipboard

Incorrect results from `isTransactionActive` call in DBAL

Open pixeloution opened this issue 11 months ago • 6 comments

Bug Report

Q A
doctrine/dbal 3.5.1

Summary

While using mySQL 8, DBAL method isTransactionActive can return false while there is no active transaction. As a result of this, I could also describe getTransactionNestingLevel() as having an incorrect value.

Current behaviour

The value of Connection::$transactionNestingLevel can get out of sync when most DDL statements are executed via executeStatement(). Executing a DDL will cause an implicit transaction and commit. Multiple statements inside a try/catch block may attempt to rollback after checking isTransactionActive() and receive a fatal error.

How to reproduce

/** @var Doctrine\DBAL\Connection $connection */
$connection->beginTransaction();

try {
    // could be ANY DDL on a non-temporary table, and most DDLs on temporary tables other than CREATE / DROP table
    $connection->executeStatement('CREATE TABLE IF NOT EXISTS SimpleTable (id INT PRIMARY KEY)');
    // uh-oh, Unknown column causes exception
    $connection->executeStatement('INSERT INTO SimpleTable (id, otherColumn) VALUES ("G", "H")');
}
catch (Throwable $e) {
    // attempting to rollback will cause a fatal error here
    if ($connection->isTransactionActive()) $connection->rollback();
}

Expected behaviour

isTransactionActive should return false if no transaction is active. You can the real status via the underlying PDO object if you're aware of this issue in DBAL.

pixeloution avatar Jul 23 '23 21:07 pixeloution

method isTransactionActive can return false while there is no active transaction

"while" often indicates a contradiction, yet there is none in your sentence. Did you mean to write true instead of false?

That being said, I don't think the DBAL can or should tell DML/DDL statements apart. With that in mind, I don't think there is a solution to this, besides documenting this MySQL weirdness, which I already kind of did in another repository: https://www.doctrine-project.org/projects/doctrine-migrations/en/stable/explanation/implicit-commits

greg0ire avatar Jul 24 '23 08:07 greg0ire

Seriously, those implicit commits are one of the biggest WTFs of MySQL and MariaDB.

And I agree with @greg0ire, I don't want to sniff SQL statements just to anticipate implicit commits. Neither do I want to continuously poll the transactional status to keep the nesting level in sync.

The best advice that I can give you is to not perform DDL operations in a transaction if you know that you're connected to a MySQL or MariaDB. It's pointless to do that anyway.

receive a fatal error

Are you sure you get a fatal error and not just an exception?

You can the real status via the underlying PDO object

We could. Fun fact: Before PHP 8, PDO::inTransaction() was prone to the same error. 🙈 🙉

The problems that I see with that approach:

  • That would only work with PDO. I'm not aware that we could poll the transactional status with MySQLi.
  • Our driver architecture does not expose an API for that. We would need to add a new method to the Connection interface just to be able to leverage a functionality that we only need to work around weird behavior of a single database system.

I'd like to turn this into a documentation issue if that's okay for you.

derrabus avatar Jul 24 '23 09:07 derrabus

And I agree with @greg0ire, I don't want to sniff SQL statements just to anticipate implicit commits. Neither do I want to continuously poll the transactional status to keep the nesting level in sync.

DDL operation can be detected quite easily by checking the first few bytes query bytes. I would expect DBAL to do it, as implicit commit might commit unwanted data. DDL operation should simply throw if in active transaction as DDL operations effectively do not support transactions.

The best advice that I can give you is to not perform DDL operations in a transaction if you know that you're connected to a MySQL or MariaDB. It's pointless to do that anyway.

yes

mvorisek avatar Aug 15 '23 08:08 mvorisek

And I agree with @greg0ire, I don't want to sniff SQL statements just to anticipate implicit commits. Neither do I want to continuously poll the transactional status to keep the nesting level in sync.

DDL operation can be detected quite easily by checking the first few bytes query bytes.

Sure, but not all of them cause an implicit commit and for some it even depends on the storage engine. This is a can of worms. I certainly do not want to build and/or maintain that detection logic.

I would expect DBAL to do it, as implicit commit might commit unwanted data. DDL operation should simply throw if in active transaction as DDL operations effectively do not support transactions.

You mean, we should suddenly start to throw on perfectly fine DDL statements just to keep the transaction level in sync? I highly doubt that it's worth it.

derrabus avatar Aug 15 '23 09:08 derrabus

Sure, but not all of them cause an implicit commit and for some it even depends on the storage engine. This is a can of worms. I certainly do not want to build and/or maintain that detection logic.

Can you please post an example when DDL statement implicit commit depends on storage engine?

ref: https://dev.mysql.com/doc/refman/8.0/en/implicit-commit.html

You mean, we should suddenly start to throw on perfectly fine DDL statements just to keep the transaction level in sync? I highly doubt that it's worth it.

yes, as opened transaction should not be commited by anything else than explicit commit

mvorisek avatar Aug 15 '23 10:08 mvorisek

Re-reading that page, it was not a DDL statement, but LOAD DATA that depends on the storage engine. For pure DDL statements that logic is actually context-free.

derrabus avatar Aug 15 '23 13:08 derrabus